Some chips such as the ENE KB9012 internal flash require a write granularity of 128 bytes.
Signed-off-by: Paul Kocialkowski contact@paulk.fr --- flash.h | 1 + flashrom.c | 6 ++++++ 2 files changed, 7 insertions(+)
diff --git a/flash.h b/flash.h index 2c2839f..24861ba 100644 --- a/flash.h +++ b/flash.h @@ -85,6 +85,7 @@ enum write_granularity { write_gran_1bit, /* Each bit can be cleared individually. */ write_gran_1byte, /* A byte can be written once. Further writes to an already written byte cause * its contents to be either undefined or to stay unchanged. */ + write_gran_128bytes, /* If less than 128 bytes are written, the unwritten bytes are undefined. */ write_gran_264bytes, /* If less than 264 bytes are written, the unwritten bytes are undefined. */ write_gran_512bytes, /* If less than 512 bytes are written, the unwritten bytes are undefined. */ write_gran_528bytes, /* If less than 528 bytes are written, the unwritten bytes are undefined. */ diff --git a/flashrom.c b/flashrom.c index d51a44c..c9c7e31 100644 --- a/flashrom.c +++ b/flashrom.c @@ -781,6 +781,9 @@ int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum break; } break; + case write_gran_128bytes: + result = need_erase_gran_bytes(have, want, len, 128); + break; case write_gran_256bytes: result = need_erase_gran_bytes(have, want, len, 256); break; @@ -847,6 +850,9 @@ static unsigned int get_next_write(const uint8_t *have, const uint8_t *want, uns case write_gran_1byte_implicit_erase: stride = 1; break; + case write_gran_128bytes: + stride = 128; + break; case write_gran_256bytes: stride = 256; break;
Most flash chips are erased to ones and programmed to zeros. However, some other flash chips, such as the ENE KB9012 internal flash, work the opposite way.
Signed-off-by: Paul Kocialkowski contact@paulk.fr --- flash.h | 3 ++- flashrom.c | 42 ++++++++++++++++++++++-------------------- 2 files changed, 24 insertions(+), 21 deletions(-)
diff --git a/flash.h b/flash.h index 24861ba..3d14d56 100644 --- a/flash.h +++ b/flash.h @@ -123,6 +123,7 @@ enum write_granularity { #define FEATURE_WRSR_EITHER (FEATURE_WRSR_EWSR | FEATURE_WRSR_WREN) #define FEATURE_OTP (1 << 8) #define FEATURE_QPI (1 << 9) +#define FEATURE_ERASED_ZERO (1 << 10)
enum test_state { OK = 0, @@ -275,7 +276,7 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f int read_flash_to_file(struct flashctx *flash, const char *filename); char *extract_param(const char *const *haystack, const char *needle, const char *delim); int verify_range(struct flashctx *flash, const uint8_t *cmpbuf, unsigned int start, unsigned int len); -int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran); +int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran, uint8_t erased); void print_version(void); void print_buildinfo(void); void print_banner(void); diff --git a/flashrom.c b/flashrom.c index c9c7e31..e463a18 100644 --- a/flashrom.c +++ b/flashrom.c @@ -669,7 +669,7 @@ static int compare_range(const uint8_t *wantbuf, const uint8_t *havebuf, unsigne
/* start is an offset to the base address of the flash chip */ int check_erased_range(struct flashctx *flash, unsigned int start, - unsigned int len) + unsigned int len, uint8_t erased) { int ret; uint8_t *cmpbuf = malloc(len); @@ -678,7 +678,7 @@ int check_erased_range(struct flashctx *flash, unsigned int start, msg_gerr("Could not allocate memory!\n"); exit(1); } - memset(cmpbuf, 0xff, len); + memset(cmpbuf, erased, len); ret = verify_range(flash, cmpbuf, start, len); free(cmpbuf); return ret; @@ -731,7 +731,7 @@ out_free: }
/* Helper function for need_erase() that focuses on granularities of gran bytes. */ -static int need_erase_gran_bytes(const uint8_t *have, const uint8_t *want, unsigned int len, unsigned int gran) +static int need_erase_gran_bytes(const uint8_t *have, const uint8_t *want, unsigned int len, unsigned int gran, uint8_t erased) { unsigned int i, j, limit; for (j = 0; j < len / gran; j++) { @@ -741,7 +741,7 @@ static int need_erase_gran_bytes(const uint8_t *have, const uint8_t *want, unsig continue; /* have needs to be in erased state. */ for (i = 0; i < limit; i++) - if (have[j * gran + i] != 0xff) + if (have[j * gran + i] != erased) return 1; } return 0; @@ -761,7 +761,7 @@ static int need_erase_gran_bytes(const uint8_t *have, const uint8_t *want, unsig * @gran write granularity (enum, not count) * @return 0 if no erase is needed, 1 otherwise */ -int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran) +int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran, uint8_t erased) { int result = 0; unsigned int i; @@ -776,31 +776,31 @@ int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum break; case write_gran_1byte: for (i = 0; i < len; i++) - if ((have[i] != want[i]) && (have[i] != 0xff)) { + if ((have[i] != want[i]) && (have[i] != erased)) { result = 1; break; } break; case write_gran_128bytes: - result = need_erase_gran_bytes(have, want, len, 128); + result = need_erase_gran_bytes(have, want, len, 128, erased); break; case write_gran_256bytes: - result = need_erase_gran_bytes(have, want, len, 256); + result = need_erase_gran_bytes(have, want, len, 256, erased); break; case write_gran_264bytes: - result = need_erase_gran_bytes(have, want, len, 264); + result = need_erase_gran_bytes(have, want, len, 264, erased); break; case write_gran_512bytes: - result = need_erase_gran_bytes(have, want, len, 512); + result = need_erase_gran_bytes(have, want, len, 512, erased); break; case write_gran_528bytes: - result = need_erase_gran_bytes(have, want, len, 528); + result = need_erase_gran_bytes(have, want, len, 528, erased); break; case write_gran_1024bytes: - result = need_erase_gran_bytes(have, want, len, 1024); + result = need_erase_gran_bytes(have, want, len, 1024, erased); break; case write_gran_1056bytes: - result = need_erase_gran_bytes(have, want, len, 1056); + result = need_erase_gran_bytes(have, want, len, 1056, erased); break; case write_gran_1byte_implicit_erase: /* Do not erase, handle content changes from anything->0xff by writing 0xff. */ @@ -1424,6 +1424,7 @@ static int erase_and_write_block_helper(struct flashctx *flash, unsigned int starthere = 0, lenhere = 0; int ret = 0, skip = 1, writecount = 0; enum write_granularity gran = flash->chip->gran; + uint8_t erased = (flash->chip->feature_bits & FEATURE_ERASED_ZERO) ? 0x00 : 0xff;
/* curcontents and newcontents are opaque to walk_eraseregions, and * need to be adjusted here to keep the impression of proper abstraction @@ -1431,17 +1432,17 @@ static int erase_and_write_block_helper(struct flashctx *flash, curcontents += start; newcontents += start; msg_cdbg(":"); - if (need_erase(curcontents, newcontents, len, gran)) { + if (need_erase(curcontents, newcontents, len, gran, erased)) { msg_cdbg("E"); ret = erasefn(flash, start, len); if (ret) return ret; - if (check_erased_range(flash, start, len)) { + if (check_erased_range(flash, start, len, erased)) { msg_cerr("ERASE FAILED!\n"); return -1; } /* Erase was successful. Adjust curcontents. */ - memset(curcontents, 0xff, len); + memset(curcontents, erased, len); skip = 0; } /* get_next_write() sets starthere to a new value after the call. */ @@ -1938,6 +1939,7 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it, int ret = 0; unsigned long size = flash->chip->total_size * 1024; int read_all_first = 1; /* FIXME: Make this configurable. */ + uint8_t erased = (flash->chip->feature_bits & FEATURE_ERASED_ZERO) ? 0x00 : 0xff;
if (chip_safety_check(flash, force, read_it, write_it, erase_it, verify_it)) { msg_cerr("Aborting.\n"); @@ -1964,15 +1966,15 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it, msg_gerr("Out of memory!\n"); exit(1); } - /* Assume worst case: All bits are 0. */ - memset(oldcontents, 0x00, size); + /* Assume worst case: All bits are not erased. */ + memset(oldcontents, ~erased, size); newcontents = malloc(size); if (!newcontents) { msg_gerr("Out of memory!\n"); exit(1); } - /* Assume best case: All bits should be 1. */ - memset(newcontents, 0xff, size); + /* Assume best case: All bits should be erased. */ + memset(newcontents, erased, size); /* Side effect of the assumptions above: Default write action is erase * because newcontents looks like a completely erased chip, and * oldcontents being completely 0x00 means we have to erase everything
On 10.10.2015 16:20, Paul Kocialkowski wrote:
Most flash chips are erased to ones and programmed to zeros. However, some other flash chips, such as the ENE KB9012 internal flash, work the opposite way.
Signed-off-by: Paul Kocialkowski contact@paulk.fr
Looks good, some comments below.
Nico
flash.h | 3 ++- flashrom.c | 42 ++++++++++++++++++++++-------------------- 2 files changed, 24 insertions(+), 21 deletions(-)
diff --git a/flash.h b/flash.h index 24861ba..3d14d56 100644 --- a/flash.h +++ b/flash.h @@ -123,6 +123,7 @@ enum write_granularity { #define FEATURE_WRSR_EITHER (FEATURE_WRSR_EWSR | FEATURE_WRSR_WREN) #define FEATURE_OTP (1 << 8) #define FEATURE_QPI (1 << 9) +#define FEATURE_ERASED_ZERO (1 << 10)
enum test_state { OK = 0, @@ -275,7 +276,7 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f int read_flash_to_file(struct flashctx *flash, const char *filename); char *extract_param(const char *const *haystack, const char *needle, const char *delim); int verify_range(struct flashctx *flash, const uint8_t *cmpbuf, unsigned int start, unsigned int len); -int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran); +int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran, uint8_t erased);
When reading `erased` I though it sounds like a boolean. Maybe call it `erased_value`?
void print_version(void); void print_buildinfo(void); void print_banner(void); diff --git a/flashrom.c b/flashrom.c index c9c7e31..e463a18 100644 --- a/flashrom.c +++ b/flashrom.c @@ -669,7 +669,7 @@ static int compare_range(const uint8_t *wantbuf, const uint8_t *havebuf, unsigne
/* start is an offset to the base address of the flash chip */ int check_erased_range(struct flashctx *flash, unsigned int start,
unsigned int len)
unsigned int len, uint8_t erased)
There's already `struct flashctx *` in the signature, doesn't that have the information this time, already?
[...]
@@ -1424,6 +1424,7 @@ static int erase_and_write_block_helper(struct flashctx *flash, unsigned int starthere = 0, lenhere = 0; int ret = 0, skip = 1, writecount = 0; enum write_granularity gran = flash->chip->gran;
- uint8_t erased = (flash->chip->feature_bits & FEATURE_ERASED_ZERO) ? 0x00 : 0xff;
You could make the transformation a macro or inline function, that would save you from writing it out in full again. Also, as this variable never changes later, it could be declared `const`. Hmmm, that also applies to the added parameters.
Hi Nico,
Le mercredi 14 octobre 2015 à 21:59 +0200, Nico Huber a écrit :
On 10.10.2015 16:20, Paul Kocialkowski wrote:
Most flash chips are erased to ones and programmed to zeros. However, some other flash chips, such as the ENE KB9012 internal flash, work the opposite way.
Signed-off-by: Paul Kocialkowski contact@paulk.fr
Looks good, some comments below.
Thanks for the review Nico, I'll submit v2 of this patch with your comments in mind.
flash.h | 3 ++- flashrom.c | 42 ++++++++++++++++++++++-------------------- 2 files changed, 24 insertions(+), 21 deletions(-)
diff --git a/flash.h b/flash.h index 24861ba..3d14d56 100644 --- a/flash.h +++ b/flash.h @@ -123,6 +123,7 @@ enum write_granularity { #define FEATURE_WRSR_EITHER (FEATURE_WRSR_EWSR | FEATURE_WRSR_WREN) #define FEATURE_OTP (1 << 8) #define FEATURE_QPI (1 << 9) +#define FEATURE_ERASED_ZERO (1 << 10)
enum test_state { OK = 0, @@ -275,7 +276,7 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f int read_flash_to_file(struct flashctx *flash, const char *filename); char *extract_param(const char *const *haystack, const char *needle, const char *delim); int verify_range(struct flashctx *flash, const uint8_t *cmpbuf, unsigned int start, unsigned int len); -int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran); +int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran, uint8_t erased);
When reading `erased` I though it sounds like a boolean. Maybe call it `erased_value`?
Good point.
void print_version(void); void print_buildinfo(void); void print_banner(void); diff --git a/flashrom.c b/flashrom.c index c9c7e31..e463a18 100644 --- a/flashrom.c +++ b/flashrom.c @@ -669,7 +669,7 @@ static int compare_range(const uint8_t *wantbuf, const uint8_t *havebuf, unsigne
/* start is an offset to the base address of the flash chip */ int check_erased_range(struct flashctx *flash, unsigned int start,
unsigned int len)
unsigned int len, uint8_t erased)There's already `struct flashctx *` in the signature, doesn't that have the information this time, already?
It does indeed, and I agree we'd better not pass extra arguments when those can be figured out from what we already have. Having a macro for erased_value will help.
[...]
@@ -1424,6 +1424,7 @@ static int erase_and_write_block_helper(struct flashctx *flash, unsigned int starthere = 0, lenhere = 0; int ret = 0, skip = 1, writecount = 0; enum write_granularity gran = flash->chip->gran;
- uint8_t erased = (flash->chip->feature_bits &
FEATURE_ERASED_ZERO) ? 0x00 : 0xff;
You could make the transformation a macro or inline function, that would save you from writing it out in full again. Also, as this variable never changes later, it could be declared `const`. Hmmm, that also applies to the added parameters.
Agreed, I will make those changes, probably with a macro.
The ENE Embedded Debug Interface (EDI) is a SPI-based interface for accessing the memory of ENE embedded controllers.
The ENE KB9012 EC is an embedded controller found on various laptops such as the Lenovo G505s. It features a 8051 microcontroller and has 128 KiB of internal storage for program data.
EDI can be accessed on the KB9012 through pins 59-62 (CS-CLK-MOSI-MISO) when flash direct access is not in use. Some firmwares disable EDI at run-time, so it might be necessary to ground pin 42 to reset the 8051 microcontroller before accessing the KB9012 via EDI.
Signed-off-by: Paul Kocialkowski contact@paulk.fr --- Makefile | 2 +- chipdrivers.h | 6 + edi.c | 418 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ edi.h | 34 +++++ ene.h | 55 ++++++++ flashchips.c | 23 ++++ 6 files changed, 537 insertions(+), 1 deletion(-) create mode 100644 edi.c create mode 100644 edi.h create mode 100644 ene.h
diff --git a/Makefile b/Makefile index c439d8d..661c52a 100644 --- a/Makefile +++ b/Makefile @@ -368,7 +368,7 @@ endif
CHIP_OBJS = jedec.o stm50.o w39.o w29ee011.o \ sst28sf040.o 82802ab.o \ - sst49lfxxxc.o sst_fwhub.o flashchips.o spi.o spi25.o spi25_statusreg.o \ + sst49lfxxxc.o sst_fwhub.o edi.o flashchips.o spi.o spi25.o spi25_statusreg.o \ opaque.o sfdp.o en29lv640b.o at45db.o
############################################################################### diff --git a/chipdrivers.h b/chipdrivers.h index cac94f3..8015b52 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -194,4 +194,10 @@ int erase_sector_stm50(struct flashctx *flash, unsigned int block, unsigned int int probe_en29lv640b(struct flashctx *flash); int write_en29lv640b(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len);
+/* edi.c */ +int edi_chip_block_erase(struct flashctx *flash, unsigned int page, unsigned int size); +int edi_chip_write(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); +int edi_chip_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); +int edi_probe_kb9012(struct flashctx *flash); + #endif /* !__CHIPDRIVERS_H__ */ diff --git a/edi.c b/edi.c new file mode 100644 index 0000000..a3e0539 --- /dev/null +++ b/edi.c @@ -0,0 +1,418 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2015 Paul Kocialkowski contact@paulk.fr + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include <string.h> +#include "flash.h" +#include "ene.h" +#include "edi.h" + +static unsigned int edi_read_buffer_length = EDI_READ_BUFFER_LENGTH_DEFAULT; + +static struct ene_chip ene_kb9012 = { + .hwversion = ENE_KB9012_HWVERSION, + .ediid = ENE_KB9012_EDIID, +}; + +static void edi_write_cmd(unsigned char *cmd, unsigned short address, unsigned char data) +{ + cmd[0] = EDI_WRITE; /* EDI write command. */ + cmd[1] = 0x00; /* Address is only 2 bytes. */ + cmd[2] = (address >> 8) & 0xff; /* Address higher byte. */ + cmd[3] = (address >> 0) & 0xff; /* Address lower byte. */ + cmd[4] = data; /* Write data. */ +} + +static void edi_read_cmd(unsigned char *cmd, unsigned short address) +{ + cmd[0] = EDI_READ; /* EDI read command. */ + cmd[1] = 0x00; /* Address is only 2 bytes. */ + cmd[2] = (address >> 8) & 0xff; /* Address higher byte. */ + cmd[3] = (address >> 0) & 0xff; /* Address lower byte. */ +} + +static int edi_write(struct flashctx *flash, unsigned short address, unsigned char data) +{ + unsigned char cmd[5] = { 0 }; + int rc; + + edi_write_cmd((unsigned char *)cmd, address, data); + + rc = spi_send_command(flash, sizeof(cmd), 0, (unsigned char *)cmd, NULL); + if (rc) + return rc; + + return 0; +} + +static int edi_read(struct flashctx *flash, unsigned short address, unsigned char *data) +{ + unsigned char cmd[4] = { 0 }; + unsigned char buffer[edi_read_buffer_length]; + unsigned int i; + int rc; + + edi_read_cmd((unsigned char *)cmd, address); + + memset(buffer, 0, sizeof(buffer)); + + rc = spi_send_command(flash, sizeof(cmd), sizeof(buffer), (unsigned char *)cmd, (unsigned char *)buffer); + if (rc) + return rc; + + for (i = 0; i < sizeof(buffer); i++) { + if (buffer[i] == EDI_NOT_READY) + continue; + + if (buffer[i] == EDI_READY) { + if (i == (sizeof(buffer) - 1)) { + /* + * Buffer size was too small for receiving the value. + * This is as good as getting only EDI_NOT_READY. + */ + + buffer[i] = EDI_NOT_READY; + break; + } + + *data = buffer[i + 1]; + return 0; + } + } + + if (buffer[i - 1] == EDI_NOT_READY) { + /* + * Buffer size is increased, one step at a time, + * to hold more data if we only catch EDI_NOT_READY. + * Once CS is deasserted, no more data will be sent by the EC, + * so we cannot keep reading afterwards and have to start a new + * transaction with a longer buffer, to be safe. + */ + + if ((edi_read_buffer_length + 1) <= EDI_READ_BUFFER_LENGTH_MAX) { + msg_pwarn("%s: Retrying read with greater buffer length!\n", __func__); + edi_read_buffer_length++; + + return edi_read(flash, address, data); + } else { + msg_perr("%s: Maximum buffer length reached and data still not ready!\n", __func__); + return -1; + } + } + + return -1; +} + +static int edi_chip_probe(struct flashctx *flash, struct ene_chip *chip) +{ + unsigned char hwversion = 0; + unsigned char ediid = 0; + int rc; + + rc = edi_read(flash, ENE_EC_HWVERSION, &hwversion); + if (rc < 0) + return 0; + + rc = edi_read(flash, ENE_EC_EDIID, &ediid); + if (rc < 0) + return 0; + + if (chip->hwversion == hwversion && chip->ediid == ediid) + return 1; + + return 0; +} + +static int edi_disable(struct flashctx *flash) +{ + unsigned char cmd = EDI_DISABLE; + int rc; + + rc = spi_send_command(flash, sizeof(cmd), 0, (unsigned char *)&cmd, NULL); + if (rc) + return rc; + + return 0; +} + +static int edi_spi_enable(struct flashctx *flash) +{ + unsigned char buffer = 0; + int rc; + + rc = edi_read(flash, ENE_XBI_EFCFG, &buffer); + if (rc < 0) + return rc; + + buffer |= ENE_XBI_EFCFG_CMD_WE; + + rc = edi_write(flash, ENE_XBI_EFCFG, buffer); + if (rc < 0) + return rc; + + return 0; +} + +static int edi_spi_disable(struct flashctx *flash) +{ + unsigned char buffer = 0; + int rc; + + rc = edi_read(flash, ENE_XBI_EFCFG, &buffer); + if (rc < 0) + return rc; + + buffer &= ~ENE_XBI_EFCFG_CMD_WE; + + rc = edi_write(flash, ENE_XBI_EFCFG, buffer); + if (rc < 0) + return rc; + + return 0; +} + +static int edi_spi_busy(struct flashctx *flash) +{ + unsigned char buffer = 0; + int rc; + + rc = edi_read(flash, ENE_XBI_EFCFG, &buffer); + if (rc < 0) + return rc; + + return !!(buffer & ENE_XBI_EFCFG_BUSY); +} + +static int edi_8051_reset(struct flashctx *flash) +{ + unsigned char buffer = 0; + int rc; + + rc = edi_read(flash, ENE_EC_PXCFG, &buffer); + if (rc < 0) + return rc; + + buffer |= ENE_EC_PXCFG_8051_RESET; + + rc = edi_write(flash, ENE_EC_PXCFG, buffer); + if (rc < 0) + return rc; + + return 0; +} + +static int edi_8051_execute(struct flashctx *flash) +{ + unsigned char buffer = 0; + int rc; + + rc = edi_read(flash, ENE_EC_PXCFG, &buffer); + if (rc < 0) + return rc; + + buffer &= ~ENE_EC_PXCFG_8051_RESET; + + rc = edi_write(flash, ENE_EC_PXCFG, buffer); + if (rc < 0) + return rc; + + return 0; +} + +int edi_chip_block_erase(struct flashctx *flash, unsigned int page, unsigned int size) +{ + unsigned int timeout = 64; + + if (size != flash->chip->page_size) { + msg_perr("%s: Block erase size is not page size!\n", __func__); + return -1; + } + + edi_spi_enable(flash); + + edi_write(flash, ENE_XBI_EFA0, ((page & 0xff) >> 0)); + edi_write(flash, ENE_XBI_EFA1, ((page & 0xff00) >> 8)); + edi_write(flash, ENE_XBI_EFA2, ((page & 0xff0000) >> 16)); + + edi_write(flash, ENE_XBI_EFCMD, ENE_XBI_EFCMD_ERASE); + + while (edi_spi_busy(flash) && timeout--) + programmer_delay(10); + + edi_spi_disable(flash); + + return 0; +} + +int edi_chip_write(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len) +{ + unsigned int address = start; + unsigned int pages; + unsigned int timeout; + unsigned int i, j; + + if ((start % flash->chip->page_size) != 0) { + msg_perr("%s: Start address is not page-aligned!\n", __func__); + return -1; + } + + if ((len % flash->chip->page_size) != 0) { + msg_perr("%s: Length is not page-aligned!\n", __func__); + return -1; + } + + pages = len / flash->chip->page_size; + + edi_spi_enable(flash); + + edi_write(flash, ENE_XBI_EFA0, ((address & 0xff) >> 0)); + edi_write(flash, ENE_XBI_EFA1, ((address & 0xff00) >> 8)); + edi_write(flash, ENE_XBI_EFA2, ((address & 0xff0000) >> 16)); + + for (i = 0; i < pages; i++) { + timeout = 64; + + /* Clear page buffer. */ + edi_write(flash, ENE_XBI_EFCMD, ENE_XBI_EFCMD_HVPL_CLEAR); + + for (j = 0; j < flash->chip->page_size; j++) { + if ((address - start) > 0) { + if (((address - 1) & 0xff) != (address & 0xff)) + edi_write(flash, ENE_XBI_EFA0, ((address & 0xff) >> 0)); + if (((address - 1) & 0xff00) != (address & 0xff00)) + edi_write(flash, ENE_XBI_EFA1, ((address & 0xff00) >> 8)); + if (((address - 1) & 0xff0000) != (address & 0xff0000)) + edi_write(flash, ENE_XBI_EFA2, ((address & 0xff0000) >> 16)); + } + + edi_write(flash, ENE_XBI_EFDAT, *buf); + edi_write(flash, ENE_XBI_EFCMD, ENE_XBI_EFCMD_HVPL_LATCH); + + buf++; + address++; + } + + /* Program page buffer to flash. */ + edi_write(flash, ENE_XBI_EFCMD, ENE_XBI_EFCMD_PROGRAM); + + while (edi_spi_busy(flash) && timeout--) + programmer_delay(10); + } + + edi_spi_disable(flash); + + return 0; +} + +int edi_chip_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) +{ + unsigned int address = start; + unsigned int i; + unsigned int timeout; + int rc; + + edi_spi_enable(flash); + + edi_write(flash, ENE_XBI_EFA0, ((address & 0xff) >> 0)); + edi_write(flash, ENE_XBI_EFA1, ((address & 0xff00) >> 8)); + edi_write(flash, ENE_XBI_EFA2, ((address & 0xff0000) >> 16)); + + /* + * EDI brings such a drastic overhead that there is about no need to + * have any delay in between calls. The EDI protocol will handle wait + * I/O times on its own anyway. + */ + + for (i = 0; i < len; i++) { + timeout = 64; + + if ((address - start) > 0) { + if (((address - 1) & 0xff) != (address & 0xff)) + edi_write(flash, ENE_XBI_EFA0, ((address & 0xff) >> 0)); + if (((address - 1) & 0xff00) != (address & 0xff00)) + edi_write(flash, ENE_XBI_EFA1, ((address & 0xff00) >> 8)); + if (((address - 1) & 0xff0000) != (address & 0xff0000)) + edi_write(flash, ENE_XBI_EFA2, ((address & 0xff0000) >> 16)); + } + + edi_write(flash, ENE_XBI_EFCMD, ENE_XBI_EFCMD_READ); + + do { + rc = edi_read(flash, ENE_XBI_EFDAT, buf); + if (!rc) + break; + + /* Just in case. */ + while (edi_spi_busy(flash) && timeout--) + programmer_delay(10); + } while (rc); + + buf++; + address++; + } + + edi_spi_disable(flash); + + return 0; +} + +int edi_shutdown(void *data) +{ + struct flashctx *flash; + int rc; + + if (data == NULL) + return -1; + + flash = (struct flashctx *)data; + + rc = edi_8051_execute(flash); + if (rc < 0) { + msg_perr("%s: Unable to execute 8051!\n", __func__); + return -1; + } + + rc = edi_disable(flash); + if (rc < 0) { + msg_perr("%s: Unable to disable EDI!\n", __func__); + return -1; + } + + return 0; +} + +int edi_probe_kb9012(struct flashctx *flash) +{ + int probe; + int rc; + + probe = edi_chip_probe(flash, &ene_kb9012); + if (!probe) + return 0; + + rc = edi_8051_reset(flash); + if (rc < 0) { + msg_perr("%s: Unable to reset 8051!\n", __func__); + return 0; + } + + register_shutdown(edi_shutdown, (void *)flash); + + return 1; +} diff --git a/edi.h b/edi.h new file mode 100644 index 0000000..d9387b1 --- /dev/null +++ b/edi.h @@ -0,0 +1,34 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2015 Paul Kocialkowski contact@paulk.fr + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#ifndef __EDI_H__ +#define __EDI_H__ 1 + +#define EDI_READ 0x30 +#define EDI_WRITE 0x40 +#define EDI_DISABLE 0xf3 + +#define EDI_NOT_READY 0x5f +#define EDI_READY 0x50 + +#define EDI_READ_BUFFER_LENGTH_DEFAULT 3 +#define EDI_READ_BUFFER_LENGTH_MAX 32 + +#endif diff --git a/ene.h b/ene.h new file mode 100644 index 0000000..445d28b --- /dev/null +++ b/ene.h @@ -0,0 +1,55 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2015 Paul Kocialkowski contact@paulk.fr + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#ifndef __ENE_H__ +#define __ENE_H__ 1 + +#define ENE_XBI_EFA0 0xfea8 +#define ENE_XBI_EFA1 0xfea9 +#define ENE_XBI_EFA2 0xfeaa +#define ENE_XBI_EFDAT 0xfeab +#define ENE_XBI_EFCMD 0xfeac +#define ENE_XBI_EFCFG 0xfead + +#define ENE_XBI_EFCFG_CMD_WE (1 << 3) +#define ENE_XBI_EFCFG_BUSY (1 << 1) + +#define ENE_XBI_EFCMD_HVPL_LATCH 0x02 +#define ENE_XBI_EFCMD_READ 0x03 +#define ENE_XBI_EFCMD_ERASE 0x20 +#define ENE_XBI_EFCMD_PROGRAM 0x70 +#define ENE_XBI_EFCMD_HVPL_CLEAR 0x80 + +#define ENE_EC_PXCFG 0xff14 + +#define ENE_EC_PXCFG_8051_RESET 0x01 + +#define ENE_EC_HWVERSION 0xff00 +#define ENE_EC_EDIID 0xff24 + +#define ENE_KB9012_HWVERSION 0xc3 +#define ENE_KB9012_EDIID 0x04 + +struct ene_chip { + unsigned char hwversion; + unsigned char ediid; +}; + +#endif diff --git a/flashchips.c b/flashchips.c index 574ad74..13f0574 100644 --- a/flashchips.c +++ b/flashchips.c @@ -3201,6 +3201,29 @@ const struct flashchip flashchips[] = { },
{ + .vendor = "ENE", + .name = "KB9012 (EDI)", + .bustype = BUS_SPI, + .total_size = 128, + .page_size = 128, + .feature_bits = FEATURE_ERASED_ZERO, + .tested = TEST_OK_PREW, + .probe = edi_probe_kb9012, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {128, 1024} }, + .block_erase = edi_chip_block_erase, + }, + }, + .gran = write_gran_128bytes, + .write = edi_chip_write, + .read = edi_chip_read, + .voltage = {2700, 3600}, + }, + + { .vendor = "ESMT", .name = "F49B002UA", .bustype = BUS_PARALLEL,
On 10.10.2015 16:20, Paul Kocialkowski wrote:
The ENE Embedded Debug Interface (EDI) is a SPI-based interface for accessing the memory of ENE embedded controllers.
The ENE KB9012 EC is an embedded controller found on various laptops such as the Lenovo G505s. It features a 8051 microcontroller and has 128 KiB of internal storage for program data.
EDI can be accessed on the KB9012 through pins 59-62 (CS-CLK-MOSI-MISO) when flash direct access is not in use. Some firmwares disable EDI at run-time, so it might be necessary to ground pin 42 to reset the 8051 microcontroller before accessing the KB9012 via EDI.
Signed-off-by: Paul Kocialkowski contact@paulk.fr
Thank you for taking the time to write a clean implementation. I had a good time reading it and learning about that EDI protocol :) I don't know the hardware, so I've only commented on general stuff. It looks good on the easy to test, positive paths. Failure handling OTOH needs more work.
Nico
Makefile | 2 +- chipdrivers.h | 6 + edi.c | 418 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ edi.h | 34 +++++ ene.h | 55 ++++++++ flashchips.c | 23 ++++ 6 files changed, 537 insertions(+), 1 deletion(-) create mode 100644 edi.c create mode 100644 edi.h create mode 100644 ene.h
diff --git a/Makefile b/Makefile index c439d8d..661c52a 100644 --- a/Makefile +++ b/Makefile @@ -368,7 +368,7 @@ endif
CHIP_OBJS = jedec.o stm50.o w39.o w29ee011.o \ sst28sf040.o 82802ab.o \
- sst49lfxxxc.o sst_fwhub.o flashchips.o spi.o spi25.o spi25_statusreg.o \
- sst49lfxxxc.o sst_fwhub.o edi.o flashchips.o spi.o spi25.o spi25_statusreg.o \ opaque.o sfdp.o en29lv640b.o at45db.o
############################################################################### diff --git a/chipdrivers.h b/chipdrivers.h index cac94f3..8015b52 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -194,4 +194,10 @@ int erase_sector_stm50(struct flashctx *flash, unsigned int block, unsigned int int probe_en29lv640b(struct flashctx *flash); int write_en29lv640b(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len);
+/* edi.c */ +int edi_chip_block_erase(struct flashctx *flash, unsigned int page, unsigned int size); +int edi_chip_write(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); +int edi_chip_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); +int edi_probe_kb9012(struct flashctx *flash);
#endif /* !__CHIPDRIVERS_H__ */ diff --git a/edi.c b/edi.c new file mode 100644 index 0000000..a3e0539 --- /dev/null +++ b/edi.c @@ -0,0 +1,418 @@ +/*
- This file is part of the flashrom project.
- Copyright (C) 2015 Paul Kocialkowski contact@paulk.fr
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 2 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
Please drop that last paragraph from all new files. FSF's address used to change and might again.
- */
+#include <string.h> +#include "flash.h" +#include "ene.h" +#include "edi.h"
+static unsigned int edi_read_buffer_length = EDI_READ_BUFFER_LENGTH_DEFAULT;
+static struct ene_chip ene_kb9012 = {
- .hwversion = ENE_KB9012_HWVERSION,
- .ediid = ENE_KB9012_EDIID,
+};
Could be `const` from what I've seen.
+static void edi_write_cmd(unsigned char *cmd, unsigned short address, unsigned char data) +{
- cmd[0] = EDI_WRITE; /* EDI write command. */
- cmd[1] = 0x00; /* Address is only 2 bytes. */
- cmd[2] = (address >> 8) & 0xff; /* Address higher byte. */
- cmd[3] = (address >> 0) & 0xff; /* Address lower byte. */
- cmd[4] = data; /* Write data. */
+}
+static void edi_read_cmd(unsigned char *cmd, unsigned short address) +{
- cmd[0] = EDI_READ; /* EDI read command. */
- cmd[1] = 0x00; /* Address is only 2 bytes. */
- cmd[2] = (address >> 8) & 0xff; /* Address higher byte. */
- cmd[3] = (address >> 0) & 0xff; /* Address lower byte. */
+}
+static int edi_write(struct flashctx *flash, unsigned short address, unsigned char data) +{
- unsigned char cmd[5] = { 0 };
edi_write_cmd() below already fully initializes `cmd`.
- int rc;
- edi_write_cmd((unsigned char *)cmd, address, data);
- rc = spi_send_command(flash, sizeof(cmd), 0, (unsigned char *)cmd, NULL);
I don't see a reason to cast `cmd`. But maybe it's just my C. Also you could just return spi_send_command(...)...
- if (rc)
return rc;- return 0;
... or return rc unconditionally. It looks very weird that way.
+}
+static int edi_read(struct flashctx *flash, unsigned short address, unsigned char *data) +{
- unsigned char cmd[4] = { 0 };
Again, `cmd` gets fully initialized later.
- unsigned char buffer[edi_read_buffer_length];
- unsigned int i;
- int rc;
- edi_read_cmd((unsigned char *)cmd, address);
- memset(buffer, 0, sizeof(buffer));
Why?
- rc = spi_send_command(flash, sizeof(cmd), sizeof(buffer), (unsigned char *)cmd, (unsigned char *)buffer);
Maybe unnecessary casts.
- if (rc)
return rc;- for (i = 0; i < sizeof(buffer); i++) {
if (buffer[i] == EDI_NOT_READY)continue;if (buffer[i] == EDI_READY) {if (i == (sizeof(buffer) - 1)) {/** Buffer size was too small for receiving the value.* This is as good as getting only EDI_NOT_READY.*/buffer[i] = EDI_NOT_READY;break;
If you break here, `i` won't get increased and `buffer[i]` is never read.
}*data = buffer[i + 1];return 0;}
So you're ignoring everything but EDI_READY and EDI_NOT_READY. Are there other valid values that might occur? Or could we just bail out? return failure here?
- }
- if (buffer[i - 1] == EDI_NOT_READY) {
/** Buffer size is increased, one step at a time,* to hold more data if we only catch EDI_NOT_READY.* Once CS is deasserted, no more data will be sent by the EC,* so we cannot keep reading afterwards and have to start a new* transaction with a longer buffer, to be safe.*/if ((edi_read_buffer_length + 1) <= EDI_READ_BUFFER_LENGTH_MAX) {
So that's equivalent to `edi_read_buffer_length < EDI_READ_BUFFER_LENGTH_MAX`.
msg_pwarn("%s: Retrying read with greater buffer length!\n", __func__);edi_read_buffer_length++;return edi_read(flash, address, data);
Oh, recursion... stack usage looks not that bad, but could you live without it? For example write a edi_retry_read() that calls a non-recur- sive edi_read() in a loop?
} else {msg_perr("%s: Maximum buffer length reached and data still not ready!\n", __func__);return -1;
You'd return -1 anyway below, but that's ok, it looks more balanced this way...
}- }
- return -1;
+}
+static int edi_chip_probe(struct flashctx *flash, struct ene_chip *chip)
`chip` could be const.
+{
- unsigned char hwversion = 0;
- unsigned char ediid = 0;
- int rc;
- rc = edi_read(flash, ENE_EC_HWVERSION, &hwversion);
- if (rc < 0)
return 0;- rc = edi_read(flash, ENE_EC_EDIID, &ediid);
- if (rc < 0)
return 0;- if (chip->hwversion == hwversion && chip->ediid == ediid)
return 1;- return 0;
+}
+static int edi_disable(struct flashctx *flash) +{
- unsigned char cmd = EDI_DISABLE;
const?
- int rc;
There's no need for a variable here. Just return spi_send_command(...).
- rc = spi_send_command(flash, sizeof(cmd), 0, (unsigned char *)&cmd, NULL);
- if (rc)
return rc;- return 0;
+}
+static int edi_spi_enable(struct flashctx *flash) +{
- unsigned char buffer = 0;
- int rc;
- rc = edi_read(flash, ENE_XBI_EFCFG, &buffer);
- if (rc < 0)
It looks odd if you know, that it won't be > 0.
return rc;- buffer |= ENE_XBI_EFCFG_CMD_WE;
- rc = edi_write(flash, ENE_XBI_EFCFG, buffer);
- if (rc < 0)
return rc;- return 0;
return edi_write(...)?
+}
+static int edi_spi_disable(struct flashctx *flash) +{
- unsigned char buffer = 0;
- int rc;
- rc = edi_read(flash, ENE_XBI_EFCFG, &buffer);
- if (rc < 0)
return rc;- buffer &= ~ENE_XBI_EFCFG_CMD_WE;
- rc = edi_write(flash, ENE_XBI_EFCFG, buffer);
- if (rc < 0)
return rc;- return 0;
+}
+static int edi_spi_busy(struct flashctx *flash) +{
- unsigned char buffer = 0;
- int rc;
- rc = edi_read(flash, ENE_XBI_EFCFG, &buffer);
- if (rc < 0)
return rc;- return !!(buffer & ENE_XBI_EFCFG_BUSY);
+}
+static int edi_8051_reset(struct flashctx *flash) +{
- unsigned char buffer = 0;
- int rc;
- rc = edi_read(flash, ENE_EC_PXCFG, &buffer);
- if (rc < 0)
return rc;- buffer |= ENE_EC_PXCFG_8051_RESET;
- rc = edi_write(flash, ENE_EC_PXCFG, buffer);
- if (rc < 0)
return rc;- return 0;
+}
+static int edi_8051_execute(struct flashctx *flash) +{
- unsigned char buffer = 0;
- int rc;
- rc = edi_read(flash, ENE_EC_PXCFG, &buffer);
- if (rc < 0)
return rc;- buffer &= ~ENE_EC_PXCFG_8051_RESET;
- rc = edi_write(flash, ENE_EC_PXCFG, buffer);
- if (rc < 0)
return rc;- return 0;
+}
+int edi_chip_block_erase(struct flashctx *flash, unsigned int page, unsigned int size)
Pedantic me: unsigned int is not assured to be wider than 16-bit ;) but that's a flashrom interface, isn't it?
+{
- unsigned int timeout = 64;
- if (size != flash->chip->page_size) {
Is this even possible? Or some kind of assertion?
msg_perr("%s: Block erase size is not page size!\n", __func__);return -1;- }
- edi_spi_enable(flash);
- edi_write(flash, ENE_XBI_EFA0, ((page & 0xff) >> 0));
- edi_write(flash, ENE_XBI_EFA1, ((page & 0xff00) >> 8));
- edi_write(flash, ENE_XBI_EFA2, ((page & 0xff0000) >> 16));
- edi_write(flash, ENE_XBI_EFCMD, ENE_XBI_EFCMD_ERASE);
Every edi_write() may fail...[1]
- while (edi_spi_busy(flash) && timeout--)
Um, edi_spi_busy() also returns true (!= 0) if it failed to read at all.
programmer_delay(10);- edi_spi_disable(flash);
- return 0;
[1]...returning 0 anyway?
+}
+int edi_chip_write(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len) +{
- unsigned int address = start;
- unsigned int pages;
- unsigned int timeout;
- unsigned int i, j;
- if ((start % flash->chip->page_size) != 0) {
msg_perr("%s: Start address is not page-aligned!\n", __func__);return -1;- }
- if ((len % flash->chip->page_size) != 0) {
msg_perr("%s: Length is not page-aligned!\n", __func__);return -1;- }
- pages = len / flash->chip->page_size;
- edi_spi_enable(flash);
- edi_write(flash, ENE_XBI_EFA0, ((address & 0xff) >> 0));
- edi_write(flash, ENE_XBI_EFA1, ((address & 0xff00) >> 8));
- edi_write(flash, ENE_XBI_EFA2, ((address & 0xff0000) >> 16));
- for (i = 0; i < pages; i++) {
timeout = 64;/* Clear page buffer. */edi_write(flash, ENE_XBI_EFCMD, ENE_XBI_EFCMD_HVPL_CLEAR);for (j = 0; j < flash->chip->page_size; j++) {if ((address - start) > 0) {
Just got confused here, so `start` is the overall start not the start of the page. Would have known that, when `start` would have been declared const ;) Also it's equivalent to `address > start`.
if (((address - 1) & 0xff) != (address & 0xff))edi_write(flash, ENE_XBI_EFA0, ((address & 0xff) >> 0));if (((address - 1) & 0xff00) != (address & 0xff00))edi_write(flash, ENE_XBI_EFA1, ((address & 0xff00) >> 8));if (((address - 1) & 0xff0000) != (address & 0xff0000))edi_write(flash, ENE_XBI_EFA2, ((address & 0xff0000) >> 16));}edi_write(flash, ENE_XBI_EFDAT, *buf);edi_write(flash, ENE_XBI_EFCMD, ENE_XBI_EFCMD_HVPL_LATCH);buf++;address++;}/* Program page buffer to flash. */edi_write(flash, ENE_XBI_EFCMD, ENE_XBI_EFCMD_PROGRAM);while (edi_spi_busy(flash) && timeout--)programmer_delay(10);- }
- edi_spi_disable(flash);
- return 0;
Again, every edi_write() may fail, edi_spi_{en,dis}able() also. Not checking on single calls might be ok, but in a sheer endless loop? that's not good. Guess, the SPI programmer driver runs into a timeout of one second for every call to spi_send_command(), you'd be waiting ages for this to end.
+}
+int edi_chip_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) +{
- unsigned int address = start;
- unsigned int i;
- unsigned int timeout;
- int rc;
- edi_spi_enable(flash);
- edi_write(flash, ENE_XBI_EFA0, ((address & 0xff) >> 0));
- edi_write(flash, ENE_XBI_EFA1, ((address & 0xff00) >> 8));
- edi_write(flash, ENE_XBI_EFA2, ((address & 0xff0000) >> 16));
- /*
* EDI brings such a drastic overhead that there is about no need to* have any delay in between calls. The EDI protocol will handle wait* I/O times on its own anyway.*/- for (i = 0; i < len; i++) {
timeout = 64;if ((address - start) > 0) {if (((address - 1) & 0xff) != (address & 0xff))edi_write(flash, ENE_XBI_EFA0, ((address & 0xff) >> 0));if (((address - 1) & 0xff00) != (address & 0xff00))edi_write(flash, ENE_XBI_EFA1, ((address & 0xff00) >> 8));if (((address - 1) & 0xff0000) != (address & 0xff0000))edi_write(flash, ENE_XBI_EFA2, ((address & 0xff0000) >> 16));}edi_write(flash, ENE_XBI_EFCMD, ENE_XBI_EFCMD_READ);do {rc = edi_read(flash, ENE_XBI_EFDAT, buf);if (!rc)break;
if (!timeout) return ...? or loop for ever!
/* Just in case. */while (edi_spi_busy(flash) && timeout--)programmer_delay(10);} while (rc);
Redundant check (rc can't be zero when we reach it) hides the endless loop. Nice try! hehe
buf++;address++;- }
- edi_spi_disable(flash);
- return 0;
+}
+int edi_shutdown(void *data) +{
- struct flashctx *flash;
- int rc;
- if (data == NULL)
return -1;- flash = (struct flashctx *)data;
- rc = edi_8051_execute(flash);
- if (rc < 0) {
msg_perr("%s: Unable to execute 8051!\n", __func__);return -1;- }
- rc = edi_disable(flash);
- if (rc < 0) {
msg_perr("%s: Unable to disable EDI!\n", __func__);return -1;- }
- return 0;
+}
+int edi_probe_kb9012(struct flashctx *flash) +{
- int probe;
- int rc;
- probe = edi_chip_probe(flash, &ene_kb9012);
- if (!probe)
return 0;- rc = edi_8051_reset(flash);
- if (rc < 0) {
msg_perr("%s: Unable to reset 8051!\n", __func__);return 0;- }
- register_shutdown(edi_shutdown, (void *)flash);
- return 1;
+} diff --git a/edi.h b/edi.h new file mode 100644 index 0000000..d9387b1 --- /dev/null +++ b/edi.h @@ -0,0 +1,34 @@ +/*
- This file is part of the flashrom project.
- Copyright (C) 2015 Paul Kocialkowski contact@paulk.fr
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 2 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
- */
+#ifndef __EDI_H__ +#define __EDI_H__ 1
+#define EDI_READ 0x30 +#define EDI_WRITE 0x40 +#define EDI_DISABLE 0xf3
+#define EDI_NOT_READY 0x5f +#define EDI_READY 0x50
+#define EDI_READ_BUFFER_LENGTH_DEFAULT 3 +#define EDI_READ_BUFFER_LENGTH_MAX 32
+#endif diff --git a/ene.h b/ene.h new file mode 100644 index 0000000..445d28b --- /dev/null +++ b/ene.h @@ -0,0 +1,55 @@ +/*
- This file is part of the flashrom project.
- Copyright (C) 2015 Paul Kocialkowski contact@paulk.fr
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 2 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
- */
+#ifndef __ENE_H__ +#define __ENE_H__ 1
+#define ENE_XBI_EFA0 0xfea8 +#define ENE_XBI_EFA1 0xfea9 +#define ENE_XBI_EFA2 0xfeaa +#define ENE_XBI_EFDAT 0xfeab +#define ENE_XBI_EFCMD 0xfeac +#define ENE_XBI_EFCFG 0xfead
+#define ENE_XBI_EFCFG_CMD_WE (1 << 3) +#define ENE_XBI_EFCFG_BUSY (1 << 1)
+#define ENE_XBI_EFCMD_HVPL_LATCH 0x02 +#define ENE_XBI_EFCMD_READ 0x03 +#define ENE_XBI_EFCMD_ERASE 0x20 +#define ENE_XBI_EFCMD_PROGRAM 0x70 +#define ENE_XBI_EFCMD_HVPL_CLEAR 0x80
+#define ENE_EC_PXCFG 0xff14
+#define ENE_EC_PXCFG_8051_RESET 0x01
+#define ENE_EC_HWVERSION 0xff00 +#define ENE_EC_EDIID 0xff24
+#define ENE_KB9012_HWVERSION 0xc3 +#define ENE_KB9012_EDIID 0x04
+struct ene_chip {
- unsigned char hwversion;
- unsigned char ediid;
+};
+#endif diff --git a/flashchips.c b/flashchips.c index 574ad74..13f0574 100644 --- a/flashchips.c +++ b/flashchips.c @@ -3201,6 +3201,29 @@ const struct flashchip flashchips[] = { },
{
.vendor = "ENE",.name = "KB9012 (EDI)",.bustype = BUS_SPI,.total_size = 128,.page_size = 128,.feature_bits = FEATURE_ERASED_ZERO,.tested = TEST_OK_PREW,.probe = edi_probe_kb9012,.probe_timing = TIMING_ZERO,.block_erasers ={{.eraseblocks = { {128, 1024} },.block_erase = edi_chip_block_erase,},},.gran = write_gran_128bytes,.write = edi_chip_write,.read = edi_chip_read,.voltage = {2700, 3600},- },
- { .vendor = "ESMT", .name = "F49B002UA", .bustype = BUS_PARALLEL,
Hi Nico,
Le mercredi 14 octobre 2015 à 23:39 +0200, Nico Huber a écrit :
On 10.10.2015 16:20, Paul Kocialkowski wrote:
The ENE Embedded Debug Interface (EDI) is a SPI-based interface for accessing the memory of ENE embedded controllers.
The ENE KB9012 EC is an embedded controller found on various laptops such as the Lenovo G505s. It features a 8051 microcontroller and has 128 KiB of internal storage for program data.
EDI can be accessed on the KB9012 through pins 59-62 (CS-CLK-MOSI -MISO) when flash direct access is not in use. Some firmwares disable EDI at run-time, so it might be necessary to ground pin 42 to reset the 8051 microcontroller before accessing the KB9012 via EDI.
Signed-off-by: Paul Kocialkowski contact@paulk.fr
Thank you for taking the time to write a clean implementation. I had a good time reading it and learning about that EDI protocol :) I don't know the hardware, so I've only commented on general stuff.
Thanks for reviewing this!
The EDI protocol is described in the KB9012 datasheet, application apendix A.5 Embedded Flash Brief Description and the internal SPI controller in part 4.14 X-Bus Interface (XBI).
It looks good on the easy to test, positive paths. Failure handling OTOH needs more work.
I will probably send v2 right away, feel free to follow up the discussion on some of these comments in there, it'll probably be easier.
Nico
Makefile | 2 +- chipdrivers.h | 6 + edi.c | 418 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ edi.h | 34 +++++ ene.h | 55 ++++++++ flashchips.c | 23 ++++ 6 files changed, 537 insertions(+), 1 deletion(-) create mode 100644 edi.c create mode 100644 edi.h create mode 100644 ene.h
diff --git a/Makefile b/Makefile index c439d8d..661c52a 100644 --- a/Makefile +++ b/Makefile @@ -368,7 +368,7 @@ endif
CHIP_OBJS = jedec.o stm50.o w39.o w29ee011.o \ sst28sf040.o 82802ab.o \
- sst49lfxxxc.o sst_fwhub.o flashchips.o spi.o spi25.o
spi25_statusreg.o \
- sst49lfxxxc.o sst_fwhub.o edi.o flashchips.o spi.o spi25.o
spi25_statusreg.o \ opaque.o sfdp.o en29lv640b.o at45db.o
################################################################## ############# diff --git a/chipdrivers.h b/chipdrivers.h index cac94f3..8015b52 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -194,4 +194,10 @@ int erase_sector_stm50(struct flashctx *flash, unsigned int block, unsigned int int probe_en29lv640b(struct flashctx *flash); int write_en29lv640b(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len);
+/* edi.c */ +int edi_chip_block_erase(struct flashctx *flash, unsigned int page, unsigned int size); +int edi_chip_write(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); +int edi_chip_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); +int edi_probe_kb9012(struct flashctx *flash);
#endif /* !__CHIPDRIVERS_H__ */ diff --git a/edi.c b/edi.c new file mode 100644 index 0000000..a3e0539 --- /dev/null +++ b/edi.c @@ -0,0 +1,418 @@ +/*
- This file is part of the flashrom project.
- Copyright (C) 2015 Paul Kocialkowski contact@paulk.fr
- This program is free software; you can redistribute it and/or
modify
- it under the terms of the GNU General Public License as
published by
- the Free Software Foundation; either version 2 of the License,
or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public
License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
02110-1301 USA
Please drop that last paragraph from all new files. FSF's address used to change and might again.
Right, perhaps it would be worth fixing this for each flashrom file, too.
- */
+#include <string.h> +#include "flash.h" +#include "ene.h" +#include "edi.h"
+static unsigned int edi_read_buffer_length = EDI_READ_BUFFER_LENGTH_DEFAULT;
+static struct ene_chip ene_kb9012 = {
- .hwversion = ENE_KB9012_HWVERSION,
- .ediid = ENE_KB9012_EDIID,
+};
Could be `const` from what I've seen.
Correct.
+static void edi_write_cmd(unsigned char *cmd, unsigned short address, unsigned char data) +{
- cmd[0] = EDI_WRITE; /* EDI write command. */
- cmd[1] = 0x00; /* Address is only 2 bytes. */
- cmd[2] = (address >> 8) & 0xff; /* Address higher byte. */
- cmd[3] = (address >> 0) & 0xff; /* Address lower byte. */
- cmd[4] = data; /* Write data. */
+}
+static void edi_read_cmd(unsigned char *cmd, unsigned short address) +{
- cmd[0] = EDI_READ; /* EDI read command. */
- cmd[1] = 0x00; /* Address is only 2 bytes. */
- cmd[2] = (address >> 8) & 0xff; /* Address higher byte. */
- cmd[3] = (address >> 0) & 0xff; /* Address lower byte. */
+}
+static int edi_write(struct flashctx *flash, unsigned short address, unsigned char data) +{
- unsigned char cmd[5] = { 0 };
edi_write_cmd() below already fully initializes `cmd`.
Right, but I see that as a good programming habit, initializing memory that will be filled by another function, because we might be using a buffer larger than what the function will fill, etc.
Of course here, it's so simple that it's 100% safe to remove the initialization, but I don't think it's a problem to keep it.
- int rc;
- edi_write_cmd((unsigned char *)cmd, address, data);
- rc = spi_send_command(flash, sizeof(cmd), 0, (unsigned
char *)cmd, NULL);
I don't see a reason to cast `cmd`. But maybe it's just my C.
That's true, it's not required at all.
Also you could just return spi_send_command(...)...
I prefer to always have a rc variable and check the return code explicitly. Actually, I prefer to use known return codes for my local (static) functions and not return the return code of the function causing the error. This is not what I did in this patch and I'll correct this.
This is simply my coding style and while there are some minor advantages (moving code blocks around more easily, only bothering once about what those functions return as good return code) and disadvantages (longer code, very redundant), I'd like to keep things this way unless there is an explicit policy in flashrom not to do this.
Also, I very much dislike checking functions' return codes with inline calls in conditional statements or when returning, it simply looks somewhat ugly to me, unless the function returns a boolean value.
- if (rc)
return rc;- return 0;
... or return rc unconditionally. It looks very weird that way.
See above.
+}
+static int edi_read(struct flashctx *flash, unsigned short address, unsigned char *data) +{
- unsigned char cmd[4] = { 0 };
Again, `cmd` gets fully initialized later.
See above.
- unsigned char buffer[edi_read_buffer_length];
- unsigned int i;
- int rc;
- edi_read_cmd((unsigned char *)cmd, address);
- memset(buffer, 0, sizeof(buffer));
Why?
Same thing here, spi_send_command *should* fill-in the whole buffer, but in case it doesn't, I'd rather have known values instead, just in case garbage turns out to take one of EDI_READY or EDI_NOT_READY.
This is probably not a hard requirement either, but seems like a good programming habit to produce reliable code to me.
- rc = spi_send_command(flash, sizeof(cmd), sizeof(buffer),
(unsigned char *)cmd, (unsigned char *)buffer);
Maybe unnecessary casts.
- if (rc)
return rc;- for (i = 0; i < sizeof(buffer); i++) {
if (buffer[i] == EDI_NOT_READY)continue;if (buffer[i] == EDI_READY) {if (i == (sizeof(buffer) - 1)) {/** Buffer size was too small forreceiving the value.
* This is as good as getting onlyEDI_NOT_READY.
*/buffer[i] = EDI_NOT_READY;break;If you break here, `i` won't get increased and `buffer[i]` is never read.
Well spotted, I missed that one. Of course, I need to increase i here. I'll probably go with another variable "index" that keeps that last value of i seen in the loop, since increasing i before breaking looks too artificial to really be straightforward.
}*data = buffer[i + 1];return 0;}So you're ignoring everything but EDI_READY and EDI_NOT_READY. Are there other valid values that might occur? Or could we just bail out? return failure here?
Well, as far as I could see from the documentation, the only valid byte should be read after EDI_READY. EDI_NOT_READY indicates that we should keep reading and other values should be treated as garbage.
I think this implementation is consistent with that. Especially, values of 0 usually occur when the kb9012 is not responding (not powered, etc) so I don't want to do anything in particular when it happens (such as increasing the buffer).
- }
- if (buffer[i - 1] == EDI_NOT_READY) {
/** Buffer size is increased, one step at a time,* to hold more data if we only catchEDI_NOT_READY.
* Once CS is deasserted, no more data will besent by the EC,
* so we cannot keep reading afterwards and haveto start a new
* transaction with a longer buffer, to be safe.*/if ((edi_read_buffer_length + 1) <=EDI_READ_BUFFER_LENGTH_MAX) {
So that's equivalent to `edi_read_buffer_length < EDI_READ_BUFFER_LENGTH_MAX`.
Quite right! This one is quite ugly.
msg_pwarn("%s: Retrying read with greaterbuffer length!\n", __func__);
edi_read_buffer_length++;return edi_read(flash, address, data);Oh, recursion... stack usage looks not that bad, but could you live without it? For example write a edi_retry_read() that calls a non -recur- sive edi_read() in a loop?
Fair enough, let's go with renaming edi_read to edi_read_byte and having edi_read wrap the call to edi_read_byte, check its rc for EDI_NOT_READY and implement the buffer increase logic in a non -recursive way.
} else {msg_perr("%s: Maximum buffer lengthreached and data still not ready!\n", __func__);
return -1;You'd return -1 anyway below, but that's ok, it looks more balanced this way...
This becomes more consistent with the new code flow suggested above.
}- }
- return -1;
+}
+static int edi_chip_probe(struct flashctx *flash, struct ene_chip *chip)
`chip` could be const.
Ack.
+{
- unsigned char hwversion = 0;
- unsigned char ediid = 0;
- int rc;
- rc = edi_read(flash, ENE_EC_HWVERSION, &hwversion);
- if (rc < 0)
return 0;- rc = edi_read(flash, ENE_EC_EDIID, &ediid);
- if (rc < 0)
return 0;- if (chip->hwversion == hwversion && chip->ediid == ediid)
return 1;- return 0;
+}
+static int edi_disable(struct flashctx *flash) +{
- unsigned char cmd = EDI_DISABLE;
const?
Would make sense, but since other definitions of cmd in edi_read/edi_write are not const, I'd prefer to have this one not be const for consistency.
- int rc;
There's no need for a variable here. Just return spi_send_command(...).
Not my coding style, see above.
- rc = spi_send_command(flash, sizeof(cmd), 0, (unsigned
char *)&cmd, NULL);
- if (rc)
return rc;- return 0;
+}
+static int edi_spi_enable(struct flashctx *flash) +{
- unsigned char buffer = 0;
- int rc;
- rc = edi_read(flash, ENE_XBI_EFCFG, &buffer);
- if (rc < 0)
It looks odd if you know, that it won't be > 0.
I don't find that surprising. Negative rc for error and zero for success is what I usually do.
return rc;- buffer |= ENE_XBI_EFCFG_CMD_WE;
- rc = edi_write(flash, ENE_XBI_EFCFG, buffer);
- if (rc < 0)
return rc;- return 0;
return edi_write(...)?
See above.
+}
+static int edi_spi_disable(struct flashctx *flash) +{
- unsigned char buffer = 0;
- int rc;
- rc = edi_read(flash, ENE_XBI_EFCFG, &buffer);
- if (rc < 0)
return rc;- buffer &= ~ENE_XBI_EFCFG_CMD_WE;
- rc = edi_write(flash, ENE_XBI_EFCFG, buffer);
- if (rc < 0)
return rc;- return 0;
+}
+static int edi_spi_busy(struct flashctx *flash) +{
- unsigned char buffer = 0;
- int rc;
- rc = edi_read(flash, ENE_XBI_EFCFG, &buffer);
- if (rc < 0)
return rc;- return !!(buffer & ENE_XBI_EFCFG_BUSY);
+}
+static int edi_8051_reset(struct flashctx *flash) +{
- unsigned char buffer = 0;
- int rc;
- rc = edi_read(flash, ENE_EC_PXCFG, &buffer);
- if (rc < 0)
return rc;- buffer |= ENE_EC_PXCFG_8051_RESET;
- rc = edi_write(flash, ENE_EC_PXCFG, buffer);
- if (rc < 0)
return rc;- return 0;
+}
+static int edi_8051_execute(struct flashctx *flash) +{
- unsigned char buffer = 0;
- int rc;
- rc = edi_read(flash, ENE_EC_PXCFG, &buffer);
- if (rc < 0)
return rc;- buffer &= ~ENE_EC_PXCFG_8051_RESET;
- rc = edi_write(flash, ENE_EC_PXCFG, buffer);
- if (rc < 0)
return rc;- return 0;
+}
+int edi_chip_block_erase(struct flashctx *flash, unsigned int page, unsigned int size)
Pedantic me: unsigned int is not assured to be wider than 16-bit ;) but that's a flashrom interface, isn't it?
Yep, I copied the prototype as-is.
+{
- unsigned int timeout = 64;
- if (size != flash->chip->page_size) {
Is this even possible? Or some kind of assertion?
I haven't seen flashrom ask for this, but it could. When that happens, some bits on the page are left unprogrammed and when the whole page is written to memory, we're losing data.
Handling that in edi.c causes way way too much overhead, so I'm just enforcing these assumptions to be safe.
msg_perr("%s: Block erase size is not pagesize!\n", __func__);
return -1;- }
- edi_spi_enable(flash);
- edi_write(flash, ENE_XBI_EFA0, ((page & 0xff) >> 0));
- edi_write(flash, ENE_XBI_EFA1, ((page & 0xff00) >> 8));
- edi_write(flash, ENE_XBI_EFA2, ((page & 0xff0000) >> 16));
- edi_write(flash, ENE_XBI_EFCMD, ENE_XBI_EFCMD_ERASE);
Every edi_write() may fail...[1]
Indeed, I have added all the rc checking at a later point when coding and apparently missed those. Good spotting!
- while (edi_spi_busy(flash) && timeout--)
Um, edi_spi_busy() also returns true (!= 0) if it failed to read at all.
Quite right, let's make this edi_spi_busy(flash) == 1 and nevermind the error just this one time.
programmer_delay(10);- edi_spi_disable(flash);
- return 0;
[1]...returning 0 anyway?
+}
+int edi_chip_write(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len) +{
- unsigned int address = start;
- unsigned int pages;
- unsigned int timeout;
- unsigned int i, j;
- if ((start % flash->chip->page_size) != 0) {
msg_perr("%s: Start address is not page-aligned!\n", __func__);
return -1;- }
- if ((len % flash->chip->page_size) != 0) {
msg_perr("%s: Length is not page-aligned!\n",__func__);
return -1;- }
- pages = len / flash->chip->page_size;
- edi_spi_enable(flash);
- edi_write(flash, ENE_XBI_EFA0, ((address & 0xff) >> 0));
- edi_write(flash, ENE_XBI_EFA1, ((address & 0xff00) >> 8));
- edi_write(flash, ENE_XBI_EFA2, ((address & 0xff0000) >>
16));
- for (i = 0; i < pages; i++) {
timeout = 64;/* Clear page buffer. */edi_write(flash, ENE_XBI_EFCMD,ENE_XBI_EFCMD_HVPL_CLEAR);
for (j = 0; j < flash->chip->page_size; j++) {if ((address - start) > 0) {Just got confused here, so `start` is the overall start not the start of the page.
Indeed. The point here is simply to check that address - 1 is within the requested range, otherwise we end up setting the address twice the first time.
Also, note that the address has to be set each time we write a byte to the page buffer. Unfortunately, it doesn't append the byte we just wrote to the page buffer.
Would have known that, when `start` would have been declared const ;)
Sure, but again, I copied the prototype as-is from struct flashchip. Feel free to change that for the whole flashrom ;)
Also it's equivalent to `address > start`.
Good point, that's more readable.
if (((address - 1) & 0xff) !=(address & 0xff))
edi_write(flash,ENE_XBI_EFA0, ((address & 0xff) >> 0));
if (((address - 1) & 0xff00) !=(address & 0xff00))
edi_write(flash,ENE_XBI_EFA1, ((address & 0xff00) >> 8));
if (((address - 1) & 0xff0000) !=(address & 0xff0000))
edi_write(flash,ENE_XBI_EFA2, ((address & 0xff0000) >> 16));
}edi_write(flash, ENE_XBI_EFDAT, *buf);edi_write(flash, ENE_XBI_EFCMD,ENE_XBI_EFCMD_HVPL_LATCH);
buf++;address++;}/* Program page buffer to flash. */edi_write(flash, ENE_XBI_EFCMD,ENE_XBI_EFCMD_PROGRAM);
while (edi_spi_busy(flash) && timeout--)programmer_delay(10);- }
- edi_spi_disable(flash);
- return 0;
Again, every edi_write() may fail, edi_spi_{en,dis}able() also. Not checking on single calls might be ok, but in a sheer endless loop? that's not good. Guess, the SPI programmer driver runs into a timeout of one second for every call to spi_send_command(), you'd be waiting ages for this to end.
Sure, that will be fixed.
+}
+int edi_chip_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) +{
- unsigned int address = start;
- unsigned int i;
- unsigned int timeout;
- int rc;
- edi_spi_enable(flash);
- edi_write(flash, ENE_XBI_EFA0, ((address & 0xff) >> 0));
- edi_write(flash, ENE_XBI_EFA1, ((address & 0xff00) >> 8));
- edi_write(flash, ENE_XBI_EFA2, ((address & 0xff0000) >>
16));
- /*
* EDI brings such a drastic overhead that there is aboutno need to
* have any delay in between calls. The EDI protocol willhandle wait
* I/O times on its own anyway.*/- for (i = 0; i < len; i++) {
timeout = 64;if ((address - start) > 0) {if (((address - 1) & 0xff) != (address &0xff))
edi_write(flash, ENE_XBI_EFA0,((address & 0xff) >> 0));
if (((address - 1) & 0xff00) != (address &0xff00))
edi_write(flash, ENE_XBI_EFA1,((address & 0xff00) >> 8));
if (((address - 1) & 0xff0000) != (address& 0xff0000))
edi_write(flash, ENE_XBI_EFA2,((address & 0xff0000) >> 16));
}edi_write(flash, ENE_XBI_EFCMD,ENE_XBI_EFCMD_READ);
do {rc = edi_read(flash, ENE_XBI_EFDAT, buf);if (!rc)break;if (!timeout) return ...? or loop for ever!
Thanks, sorry for such carelessness, there are other places where timeout is not checked (even though it's more critical in this spot).
/* Just in case. */while (edi_spi_busy(flash) && timeout--)programmer_delay(10);} while (rc);Redundant check (rc can't be zero when we reach it) hides the endless loop. Nice try! hehe
That's correct, I simply prefer to have a dummy condition loosely related to the code logic (that is never actually reached) because, well, it doesn't hurt and looks better then while (1).
If you believe it obfuscates the code, I'd be fine with removing that form.
buf++;address++;- }
- edi_spi_disable(flash);
- return 0;
+}
+int edi_shutdown(void *data) +{
- struct flashctx *flash;
- int rc;
- if (data == NULL)
return -1;- flash = (struct flashctx *)data;
- rc = edi_8051_execute(flash);
- if (rc < 0) {
msg_perr("%s: Unable to execute 8051!\n",__func__);
return -1;- }
- rc = edi_disable(flash);
- if (rc < 0) {
msg_perr("%s: Unable to disable EDI!\n",__func__);
return -1;- }
- return 0;
+}
+int edi_probe_kb9012(struct flashctx *flash) +{
- int probe;
- int rc;
- probe = edi_chip_probe(flash, &ene_kb9012);
- if (!probe)
return 0;- rc = edi_8051_reset(flash);
- if (rc < 0) {
msg_perr("%s: Unable to reset 8051!\n", __func__);return 0;- }
- register_shutdown(edi_shutdown, (void *)flash);
- return 1;
+} diff --git a/edi.h b/edi.h new file mode 100644 index 0000000..d9387b1 --- /dev/null +++ b/edi.h @@ -0,0 +1,34 @@ +/*
- This file is part of the flashrom project.
- Copyright (C) 2015 Paul Kocialkowski contact@paulk.fr
- This program is free software; you can redistribute it and/or
modify
- it under the terms of the GNU General Public License as
published by
- the Free Software Foundation; either version 2 of the License,
or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public
License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
02110-1301 USA
- */
+#ifndef __EDI_H__ +#define __EDI_H__ 1
+#define EDI_READ 0x30 +#define EDI_WRITE 0x40 +#define EDI_DISABLE 0xf3
+#define EDI_NOT_READY 0x5f +#define EDI_READY 0x50
+#define EDI_READ_BUFFER_LENGTH_DEFAULT 3 +#define EDI_READ_BUFFER_LENGTH_MAX 32
+#endif diff --git a/ene.h b/ene.h new file mode 100644 index 0000000..445d28b --- /dev/null +++ b/ene.h @@ -0,0 +1,55 @@ +/*
- This file is part of the flashrom project.
- Copyright (C) 2015 Paul Kocialkowski contact@paulk.fr
- This program is free software; you can redistribute it and/or
modify
- it under the terms of the GNU General Public License as
published by
- the Free Software Foundation; either version 2 of the License,
or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public
License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
02110-1301 USA
- */
+#ifndef __ENE_H__ +#define __ENE_H__ 1
+#define ENE_XBI_EFA0 0xfea8 +#define ENE_XBI_EFA1 0xfea9 +#define ENE_XBI_EFA2 0xfeaa +#define ENE_XBI_EFDAT 0xfeab +#define ENE_XBI_EFCMD 0xfeac +#define ENE_XBI_EFCFG 0xfead
+#define ENE_XBI_EFCFG_CMD_WE (1 << 3) +#define ENE_XBI_EFCFG_BUSY (1 << 1)
+#define ENE_XBI_EFCMD_HVPL_LATCH 0x02 +#define ENE_XBI_EFCMD_READ 0x03 +#define ENE_XBI_EFCMD_ERASE 0x20 +#define ENE_XBI_EFCMD_PROGRAM 0x70 +#define ENE_XBI_EFCMD_HVPL_CLEAR 0x80
+#define ENE_EC_PXCFG 0xff14
+#define ENE_EC_PXCFG_8051_RESET 0x01
+#define ENE_EC_HWVERSION 0xff00 +#define ENE_EC_EDIID 0xff24
+#define ENE_KB9012_HWVERSION 0xc3 +#define ENE_KB9012_EDIID 0x04
+struct ene_chip {
- unsigned char hwversion;
- unsigned char ediid;
+};
+#endif diff --git a/flashchips.c b/flashchips.c index 574ad74..13f0574 100644 --- a/flashchips.c +++ b/flashchips.c @@ -3201,6 +3201,29 @@ const struct flashchip flashchips[] = { },
{
.vendor = "ENE",.name = "KB9012 (EDI)",.bustype = BUS_SPI,.total_size = 128,.page_size = 128,.feature_bits = FEATURE_ERASED_ZERO,.tested = TEST_OK_PREW,.probe = edi_probe_kb9012,.probe_timing = TIMING_ZERO,.block_erasers ={{.eraseblocks = { {128, 1024} },.block_erase =edi_chip_block_erase,
},},.gran = write_gran_128bytes,.write = edi_chip_write,.read = edi_chip_read,.voltage = {2700, 3600},- },
- { .vendor = "ESMT", .name = "F49B002UA", .bustype = BUS_PARALLEL,
Hi Paul,
On 24.10.2015 13:19, Paul Kocialkowski wrote:
I will probably send v2 right away, feel free to follow up the discussion on some of these comments in there, it'll probably be easier.
I'll keep some of the general discussion here. Some new comments will come inline with v2.
- unsigned char buffer[edi_read_buffer_length];
- unsigned int i;
- int rc;
- edi_read_cmd((unsigned char *)cmd, address);
- memset(buffer, 0, sizeof(buffer));
Why?
Same thing here, spi_send_command *should* fill-in the whole buffer, but in case it doesn't, I'd rather have known values instead, just in case garbage turns out to take one of EDI_READY or EDI_NOT_READY.
This is probably not a hard requirement either, but seems like a good programming habit to produce reliable code to me.
What bothers me is that you imply that spi_send_command() may behave wrong. That is very defensive programming. And if spi_send_command() really does it wrong, we'd never know.
Well, that's generally why I prefer to never initialize unless it's necessary. So many errors out there that we can't see due to over initialization :-/
do {rc = edi_read(flash, ENE_XBI_EFDAT, buf);if (!rc)break;if (!timeout) return ...? or loop for ever!
Thanks, sorry for such carelessness, there are other places where timeout is not checked (even though it's more critical in this spot).
/* Just in case. */while (edi_spi_busy(flash) && timeout--)programmer_delay(10);} while (rc);Redundant check (rc can't be zero when we reach it) hides the endless loop. Nice try! hehe
That's correct, I simply prefer to have a dummy condition loosely related to the code logic (that is never actually reached) because, well, it doesn't hurt and looks better then while (1).
If you believe it obfuscates the code, I'd be fine with removing that form.
Yes, please change that. I believe, a constant condition should always be obvious.
Nico
Le dimanche 08 novembre 2015 à 15:53 +0100, Nico Huber a écrit :
Hi Paul,
On 24.10.2015 13:19, Paul Kocialkowski wrote:
I will probably send v2 right away, feel free to follow up the discussion on some of these comments in there, it'll probably be easier.
I'll keep some of the general discussion here. Some new comments will come inline with v2.
- unsigned char buffer[edi_read_buffer_length];
- unsigned int i;
- int rc;
- edi_read_cmd((unsigned char *)cmd, address);
- memset(buffer, 0, sizeof(buffer));
Why?
Same thing here, spi_send_command *should* fill-in the whole buffer, but in case it doesn't, I'd rather have known values instead, just in case garbage turns out to take one of EDI_READY or EDI_NOT_READY.
This is probably not a hard requirement either, but seems like a good programming habit to produce reliable code to me.
What bothers me is that you imply that spi_send_command() may behave wrong. That is very defensive programming. And if spi_send_command() really does it wrong, we'd never know.
So I guess the point here is that spi_send_command will let us know in its return code when it failed to fill the whole buffer. I can work with that as a hard assumption. Some functions, such as read, read at most the requested number of bytes, thus this programming habit becomes relevant.
Well, that's generally why I prefer to never initialize unless it's necessary. So many errors out there that we can't see due to over initialization :-/
At the end of the day, I prefer to have initialized data mistakenly interpreted than random garbage (but of course, that's only a fallback and it indicates that there is a problem somewhere else). I guess the question is whether we want to have a chance to spot those kinds of issues at run time by not initializing data and having better odds of spotting a hard crash or have it keep going with a fallback path. For flashrom, I'd say it's okay to have a hard fail. For other projects, where it is vital for the program to keep running no matter what, I wouldn't agree.
On 11.11.2015 16:34, Paul Kocialkowski wrote:
Le dimanche 08 novembre 2015 à 15:53 +0100, Nico Huber a écrit :
Hi Paul,
On 24.10.2015 13:19, Paul Kocialkowski wrote:
I will probably send v2 right away, feel free to follow up the discussion on some of these comments in there, it'll probably be easier.
I'll keep some of the general discussion here. Some new comments will come inline with v2.
- unsigned char buffer[edi_read_buffer_length];
- unsigned int i;
- int rc;
- edi_read_cmd((unsigned char *)cmd, address);
- memset(buffer, 0, sizeof(buffer));
Why?
Same thing here, spi_send_command *should* fill-in the whole buffer, but in case it doesn't, I'd rather have known values instead, just in case garbage turns out to take one of EDI_READY or EDI_NOT_READY.
This is probably not a hard requirement either, but seems like a good programming habit to produce reliable code to me.
What bothers me is that you imply that spi_send_command() may behave wrong. That is very defensive programming. And if spi_send_command() really does it wrong, we'd never know.
So I guess the point here is that spi_send_command will let us know in its return code when it failed to fill the whole buffer. I can work with that as a hard assumption. Some functions, such as read, read at most the requested number of bytes, thus this programming habit becomes relevant.
Pedantic me says: In that case you shouldn't look at the bytes you didn't request to read. They have the old value anyway.
Well, that's generally why I prefer to never initialize unless it's necessary. So many errors out there that we can't see due to over initialization :-/
At the end of the day, I prefer to have initialized data mistakenly interpreted than random garbage (but of course, that's only a fallback and it indicates that there is a problem somewhere else). I guess the question is whether we want to have a chance to spot those kinds of issues at run time by not initializing data and having better odds of spotting a hard crash or have it keep going with a fallback path. For flashrom, I'd say it's okay to have a hard fail. For other projects, where it is vital for the program to keep running no matter what, I wouldn't agree.
Crashing is never an option. If it's vital for a program to keep run- ning, it has to check for garbage every time it looks into a simple buf- fer of data without guarantees on its content. Sticking to the example, spi_send_command() is allowed to fill the buffer with garbage (that will be the case when there is garbage on the SPI line). Overinitialization only gives you the feeling of being safe. You're never safe!
Nico
On 10.10.2015 16:20, Paul Kocialkowski wrote:
Some chips such as the ENE KB9012 internal flash require a write granularity of 128 bytes.
Signed-off-by: Paul Kocialkowski contact@paulk.fr
clean patch, +2
flash.h | 1 + flashrom.c | 6 ++++++ 2 files changed, 7 insertions(+)
diff --git a/flash.h b/flash.h index 2c2839f..24861ba 100644 --- a/flash.h +++ b/flash.h @@ -85,6 +85,7 @@ enum write_granularity { write_gran_1bit, /* Each bit can be cleared individually. */ write_gran_1byte, /* A byte can be written once. Further writes to an already written byte cause * its contents to be either undefined or to stay unchanged. */
- write_gran_128bytes, /* If less than 128 bytes are written, the unwritten bytes are undefined. */ write_gran_264bytes, /* If less than 264 bytes are written, the unwritten bytes are undefined. */ write_gran_512bytes, /* If less than 512 bytes are written, the unwritten bytes are undefined. */ write_gran_528bytes, /* If less than 528 bytes are written, the unwritten bytes are undefined. */
diff --git a/flashrom.c b/flashrom.c index d51a44c..c9c7e31 100644 --- a/flashrom.c +++ b/flashrom.c @@ -781,6 +781,9 @@ int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum break; } break;
- case write_gran_128bytes:
result = need_erase_gran_bytes(have, want, len, 128);break;@@ -847,6 +850,9 @@ static unsigned int get_next_write(const uint8_t *have, const uint8_t *want, uns case write_gran_1byte_implicit_erase: stride = 1; break;
- case write_gran_128bytes:
stride = 128;break;
On Wed, 14 Oct 2015 21:43:04 +0200 Nico Huber nico.h@gmx.de wrote:
On 10.10.2015 16:20, Paul Kocialkowski wrote:
Some chips such as the ENE KB9012 internal flash require a write granularity of 128 bytes.
Signed-off-by: Paul Kocialkowski contact@paulk.fr
clean patch, +2
I've understood that as an acked-by... ;) Thanks to both of you. Committed in r1897.
Hi,
Le vendredi 16 octobre 2015 à 04:25 +0200, Stefan Tauner a écrit :
On Wed, 14 Oct 2015 21:43:04 +0200 Nico Huber nico.h@gmx.de wrote:
On 10.10.2015 16:20, Paul Kocialkowski wrote:
Some chips such as the ENE KB9012 internal flash require a write granularity of 128 bytes.
Signed-off-by: Paul Kocialkowski contact@paulk.fr
clean patch, +2
I've understood that as an acked-by... ;) Thanks to both of you. Committed in r1897.
Great, thanks a lot Nico for reviewing this and Stefan for merging it!
I noticed that you have reworded the commit message: I usually do not use conjugated verbs in commit headline. Does this conflict with flashrom's guidelines?
It looks a bit strange to me to have my commit worded in a way that really does not match what I naturally do!
Hi Paul,
On 18.10.2015 19:37, Paul Kocialkowski wrote:
I noticed that you have reworded the commit message: I usually do not > use conjugated verbs in commit headline. Does this conflict with >
flashrom's guidelines?
No formal guidelines, but "128 bytes write granularity support" is missing a crucial piece of information: Was this a bugfix, a new feature or something you removed? The reworded commit message has that info.
It looks a bit strange to me to have my commit worded in a way that > really does not match what I naturally do!
Regards, Carl-Daniel
Hi Carl-Daniel,
Le dimanche 18 octobre 2015 à 20:58 +0200, Carl-Daniel Hailfinger a écrit :
On 18.10.2015 19:37, Paul Kocialkowski wrote:
I noticed that you have reworded the commit message: I usually do not > use conjugated verbs in commit headline. Does this conflict with >
flashrom's guidelines?
No formal guidelines, but "128 bytes write granularity support" is missing a crucial piece of information: Was this a bugfix, a new feature or something you removed? The reworded commit message has that info.
Well, it does end with "support", which indicates that support for this was introduced, so I don't think it was confusing.
I really see commit headlines as titles, not active phrases and would like to keep my commits phrased this way.
Please let me know during patch review next time you find the commit headline not precise enough.
Thanks!
On Thu, 22 Oct 2015 08:58:53 +0200 Paul Kocialkowski contact@paulk.fr wrote:
Hi Carl-Daniel,
Le dimanche 18 octobre 2015 à 20:58 +0200, Carl-Daniel Hailfinger a écrit :
On 18.10.2015 19:37, Paul Kocialkowski wrote:
I noticed that you have reworded the commit message: I usually do not > use conjugated verbs in commit headline. Does this conflict with >
flashrom's guidelines?
No formal guidelines, but "128 bytes write granularity support" is missing a crucial piece of information: Was this a bugfix, a new feature or something you removed? The reworded commit message has that info.
Well, it does end with "support", which indicates that support for this was introduced, so I don't think it was confusing.
I really see commit headlines as titles, not active phrases and would like to keep my commits phrased this way.
Please let me know during patch review next time you find the commit headline not precise enough.
Hello Paul,
I have not replied yet because Carl-Daniel's message described the thoughts I had/have when changing commit logs pretty much exactly. Your approach on commit logs (seeing the first line as the "subject") is IMHO a defensible argument. After all, that's how git calls it as well in many places.
However, using imperative sentences is much more common and IMHO it is better. I see every commit as an entity that changes our code base... so it always *does* something and often it is interesting what the most descriptive verb is for what it is doing (e.g. add, fix, improve, refactor, ...) to better understand what the change is about. Just like Carl-Daniel explained for the commit in question.
I can not be sure that if we someone looks at the commit in three years he will remember or even know that 128 byte granularity was introduced about the time this commit happened. It is much easier to understand a commit if the subject contains a verb that makes the action clear.
Also, let's suppose we would have to revert a commit because it was really a bad idea to commit it in the first place and we can not fix it easily. The normal commit message would be «Revert "128 bytes write granularity support"» in your case vs. «Revert "Add support for 128 bytes write granularity.» in the imperative case. The issue with the subject without a verb is similar to the original. In the second case it is completely clear that the support was removed again. Without a verb it could be a fix that is reverted, an improvement, or even the revert of removing the support... it is simply not explicit enough IMHO.
Others have written about the subject as well, but I can not find a lot of texts explaining the WHY very good... this is one example:
http://chris.beams.io/posts/git-commit/#imperative
Your other patches are affected as well and I intend to rephrase them as I see fit. I hope you understand/agree now... if not I'll have another argument: the maintainers have to maintain the codebase and are the ones who read commit messages the most, hence it should be the decision of the maintainers what's written in commit messages because they are affected most by them. The submitter can simply dump the code and hope it gets merged but apart from seeing some mails with the subject during communication it does not affect his work on the project later (because there often is none... otherwise he is not merely a submitter).
Hi Stefan,
Le jeudi 22 octobre 2015 à 22:05 +0200, Stefan Tauner a écrit :
On Thu, 22 Oct 2015 08:58:53 +0200 Paul Kocialkowski contact@paulk.fr wrote:
Hi Carl-Daniel,
Le dimanche 18 octobre 2015 à 20:58 +0200, Carl-Daniel Hailfinger a écrit :
On 18.10.2015 19:37, Paul Kocialkowski wrote:
I noticed that you have reworded the commit message: I usually do not > use conjugated verbs in commit headline. Does this conflict with >
flashrom's guidelines?
No formal guidelines, but "128 bytes write granularity support" is missing a crucial piece of information: Was this a bugfix, a new feature or something you removed? The reworded commit message has that info.
Well, it does end with "support", which indicates that support for this was introduced, so I don't think it was confusing.
I really see commit headlines as titles, not active phrases and would like to keep my commits phrased this way.
Please let me know during patch review next time you find the commit headline not precise enough.
Hello Paul,
I have not replied yet because Carl-Daniel's message described the thoughts I had/have when changing commit logs pretty much exactly. Your approach on commit logs (seeing the first line as the "subject") is IMHO a defensible argument. After all, that's how git calls it as well in many places.
However, using imperative sentences is much more common and IMHO it is better. I see every commit as an entity that changes our code base... so it always *does* something and often it is interesting what the most descriptive verb is for what it is doing (e.g. add, fix, improve, refactor, ...) to better understand what the change is about. Just like Carl-Daniel explained for the commit in question.
Well, I don't see both points of view as contradictory. When introducing support for something new, I think we're not losing any information when formatting the subject line as: Feature foo support introduction instead of Introduce support for feature foo
This goes for all the things that we might want to describe: * Bug #123 fixup * Feature bar improvement * Baz code re-factoring
Most active verbs have (more or less direct) equivalent nouns that can be used.
I get the argument that we're doing something to the code base, so it makes sense to use active verbs, but for some reason, I just find that somewhat ugly. Perhaps my vision of things will change with time, but that's how I see it for now.
Also, it seems that there is a lot of disparity regarding the tense used for such verbs : Added support for X/Add support for X/Adds support for X. And I don't really see any good way to choose between those. And I hate that sort of lack of consistency, so using non-verbal sentences felt like an elegant solution.
I can not be sure that if we someone looks at the commit in three years he will remember or even know that 128 byte granularity was introduced about the time this commit happened. It is much easier to understand a commit if the subject contains a verb that makes the action clear.
Well, let's not forget that commit messages come with longer explanations that must play a role, more than saying the same as the subject. I really see the subject as a title to the longer commit message.
Also, let's suppose we would have to revert a commit because it was really a bad idea to commit it in the first place and we can not fix it easily. The normal commit message would be «Revert "128 bytes write granularity support"» in your case vs. «Revert "Add support for 128 bytes write granularity.» in the imperative case. The issue with the subject without a verb is similar to the original. In the second case it is completely clear that the support was removed again. Without a verb it could be a fix that is reverted, an improvement, or even the revert of removing the support... it is simply not explicit enough IMHO.
Well, I think that "support" implies "support introduction", but that's something else. Also, git wraps the original line around quotation marks, which also adds some clarity.
Others have written about the subject as well, but I can not find a lot of texts explaining the WHY very good... this is one example:
One very good point here is that git uses the imperative when creating a commit on its own (Revert, Merge, etc). That sounds reasonable enough as a guideline for me to switch my behaviour.
Your other patches are affected as well and I intend to rephrase them as I see fit. I hope you understand/agree now... if not I'll have another argument: the maintainers have to maintain the codebase and are the ones who read commit messages the most, hence it should be the decision of the maintainers what's written in commit messages because they are affected most by them. The submitter can simply dump the code and hope it gets merged but apart from seeing some mails with the subject during communication it does not affect his work on the project later (because there often is none... otherwise he is not merely a submitter).
Well, that makes sense, yes, but keep in mind that it's also part of the author's contribution, so I'd like things to be discussed beforehand, just like it is somewhat rude to change a patch the way you like and merge it without discussing it with its author through code review first.
Let's say that I wouldn't object to having commit subjects rephrased the way git does it internally.
Thanks for taking the time to explain all this.
Paul Kocialkowski
-
Carl-Daniel Hailfinger -
Nico Huber -
Paul Kocialkowski -
Stefan Tauner