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 <[email protected]> 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 <[email protected]> > + * > + * 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 <[email protected]> > + * > + * 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 <[email protected]> > + * > + * 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, > _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
