[flashrom] [PATCH 3/3] ENE Embedded Debug Interface (EDI) and ENE KB9012 EC internal flash support
Nico Huber
nico.h at gmx.de
Wed Oct 14 23:39:06 CEST 2015
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 at 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 at 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 at 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 at 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,
>
More information about the flashrom
mailing list