- flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: linux_mtd: Import driver from ChromiumOS
by David Hendricks (Code Review) 17 May '18

17 May '18

David Hendricks has submitted this change and it was merged. ( https://review.coreboot.org/25706 ) Change subject: linux_mtd: Import driver from ChromiumOS ...................................................................... linux_mtd: Import driver from ChromiumOS This imports a series of patches from chromiumos for MTD support. The patches are squashed to ease review and original Change-Ids have been removed to avoid confusing Gerrit. There are a few changes to integrate the code: - Conflict resolution - Makefile changes - Remove file library usage from linux_mtd. We may revisit this and use it for other Linux interfaces later on. - Switch to using file stream functions for reads and writes. This consolidated patch is Signed-off-by: David Hendricks <dhendricks(a)fb.com> The first commit's message is: Initial MTD support This adds MTD support to flashrom so that we can read, erase, and write content on a NOR flash chip via MTD. BUG=chrome-os-partner:40208 BRANCH=none TEST=read, write, and erase works on Oak Signed-off-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/272983 Reviewed-by: Shawn N <shawnn(a)chromium.org> This is the 2nd commit message: linux_mtd: Fix compilation errors This fixes compilation errors from the initial import patch. Signed-off-by: David Hendricks <dhendricks(a)fb.com> This is the 3rd commit message: linux_mtd: Suppress message if NOR device not found This just suppresses a message that might cause confusion for unsuspecting users. BUG=none BRANCH=none TEST=ran on veyron_mickey, "NOR type device not found" message no longer appears under normal circumstances. Signed-off-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/302145 Commit-Ready: David Hendricks <dhendrix(a)chromium.org> Tested-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-by: Shawn N <shawnn(a)chromium.org> This is the 4th commit message: linux_mtd: Support for NO_ERASE type devices Some mtd devices have the MTD_NO_ERASE flag set. This means these devices don't require an erase to write and might not have implemented an erase function. We should be conservative and skip erasing altogether, falling back to performing writes over the whole flash. BUG=b:35104688 TESTED=Zaius flash is now written correctly for the 0xff regions. Signed-off-by: William A. Kennington III <wak(a)google.com> Reviewed-on: https://chromium-review.googlesource.com/472128 Commit-Ready: William Kennington <wak(a)google.com> Tested-by: William Kennington <wak(a)google.com> Reviewed-by: Brian Norris <briannorris(a)chromium.org> This is the 5th commit message: linux_mtd: do reads in eraseblock-sized chunks It's probably not the best idea to try to do an 8MB read in one syscall. Theoretically, this should work; but MTD just relies on the SPI driver to deliver the whole read in one transfer, and many SPI drivers haven't been tested well with large transfer sizes. I'd consider this a workaround, but it's still good to have IMO. BUG=chrome-os-partner:53215 TEST=boot kevin; `flashrom --read ...` TEST=check for performance regression on oak BRANCH=none Signed-off-by: Brian Norris <briannorris(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/344006 Reviewed-by: David Hendricks <dhendrix(a)chromium.org> This is the 6th commit message: linux_mtd: make read/write loop chunks consistent, and documented Theoretically, there should be no maximum size for the read() and write() syscalls on an MTD (well, except for the size of the entire device). But practical concerns (i.e., bugs) have meant we don't quite do this. For reads: Bug https://b/35573113 shows that some SPI-based MTD drivers don't yet handle very large transactions. So we artificially limit this to block-sized chunks. For writes: It's not clear there is a hard limit. Some drivers will already split large writes into smaller chunks automatically. Others don't do any splitting. At any rate, using *small* chunks can actually be a problem for some devices (b:35104688), as they get worse performance (doing an internal read/modify/write). This could be fixed in other ways by advertizing their true "write chunk size" to user space somehow, but this isn't so easy. As a simpler fix, we can just increase the loop increment to match the read loop. Per David, the original implementation (looping over page chunks) was just being paranoid. So this patch: * clarifies comments in linux_mtd_read(), to note that the chunking is somewhat of a hack that ideally can be fixed (with bug reference) * simplifies the linux_mtd_write() looping to match the structure in linux_mtd_read(), including dropping several unnecessary seeks, and correcting the error messages (they referred to "reads" and had the wrong parameters) * change linux_mtd_write() to align its chunks to eraseblocks, not page sizes Note that the "->page_size" parameter is still somewhat ill-defined, and only set by the upper layers for "opaque" flash. And it's not actually used in this driver now. If we could figure out what we really want to use it for, then we could try to set it appropriately. BRANCH=none BUG=b:35104688 TEST=various flashrom tests on Kevin TEST=Reading and writing to flash works on our zaius machines over mtd Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Signed-off-by: Brian Norris <briannorris(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/505409 Reviewed-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-by: Martin Roth <martinroth(a)chromium.org> Reviewed-by: William Kennington <wak(a)google.com> Reviewed-on: https://review.coreboot.org/25706 Tested-by: David Hendricks <david.hendricks(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> Reviewed-by: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> --- M Makefile M flash.h M flashrom.8.tmpl M flashrom.c M internal.c A linux_mtd.c M programmer.h 7 files changed, 475 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified David Hendricks: Verified Nico Huber: Looks good to me, approved Philipp Deppenwiese: Looks good to me, approved diff --git a/Makefile b/Makefile index d150dcb..943d88d 100644 --- a/Makefile +++ b/Makefile @@ -341,6 +341,11 @@ ifneq ($(TARGET_OS), Linux) # Android is handled internally as separate OS, but it supports CONFIG_LINUX_SPI and CONFIG_MSTARDDC_SPI ifneq ($(TARGET_OS), Android) +ifeq ($(CONFIG_LINUX_MTD), yes) +UNSUPPORTED_FEATURES += CONFIG_LINUX_MTD=yes +else +override CONFIG_LINUX_MTD = no +endif ifeq ($(CONFIG_LINUX_SPI), yes) UNSUPPORTED_FEATURES += CONFIG_LINUX_SPI=yes else @@ -624,7 +629,8 @@ # Always enable Marvell SATA controllers for now. CONFIG_SATAMV ?= yes -# Enable Linux spidev interface by default. We disable it on non-Linux targets. +# Enable Linux spidev and MTD interfaces by default. We disable them on non-Linux targets. +CONFIG_LINUX_MTD ?= yes CONFIG_LINUX_SPI ?= yes # Always enable ITE IT8212F PATA controllers for now. @@ -902,6 +908,12 @@ NEED_LIBPCI += CONFIG_SATAMV endif +ifeq ($(CONFIG_LINUX_MTD), yes) +# This is a totally ugly hack. +FEATURE_CFLAGS += $(call debug_shell,grep -q "LINUX_MTD_SUPPORT := yes" .features && printf "%s" "-D'CONFIG_LINUX_MTD=1'") +PROGRAMMER_OBJS += linux_mtd.o +endif + ifeq ($(CONFIG_LINUX_SPI), yes) # This is a totally ugly hack. FEATURE_CFLAGS += $(call debug_shell,grep -q "LINUX_SPI_SUPPORT := yes" .features && printf "%s" "-D'CONFIG_LINUX_SPI=1'") @@ -1277,6 +1289,18 @@ endef export UTSNAME_TEST +define LINUX_MTD_TEST +#include <mtd/mtd-user.h> + +int main(int argc, char **argv) +{ + (void) argc; + (void) argv; + return 0; +} +endef +export LINUX_MTD_TEST + define LINUX_SPI_TEST #include <linux/types.h> #include <linux/spi/spidev.h> @@ -1333,6 +1357,15 @@ ( echo "not found."; echo "FTDISUPPORT := no" >> .features.tmp ) } \ 2>>$(BUILD_DETAILS_FILE) | tee -a $(BUILD_DETAILS_FILE) endif +ifeq ($(CONFIG_LINUX_MTD), yes) + @printf "Checking if Linux MTD headers are present... " | tee -a $(BUILD_DETAILS_FILE) + @echo "$$LINUX_MTD_TEST" > .featuretest.c + @printf "\nexec: %s\n" "$(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) .featuretest.c -o .featuretest$(EXEC_SUFFIX)" >>$(BUILD_DETAILS_FILE) + @ { $(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) .featuretest.c -o .featuretest$(EXEC_SUFFIX) >&2 && \ + ( echo "yes."; echo "LINUX_MTD_SUPPORT := yes" >> .features.tmp ) || \ + ( echo "no."; echo "LINUX_MTD_SUPPORT := no" >> .features.tmp ) } \ + 2>>$(BUILD_DETAILS_FILE) | tee -a $(BUILD_DETAILS_FILE) +endif ifeq ($(CONFIG_LINUX_SPI), yes) @printf "Checking if Linux SPI headers are present... " | tee -a $(BUILD_DETAILS_FILE) @echo "$$LINUX_SPI_TEST" > .featuretest.c diff --git a/flash.h b/flash.h index 72d345a..fca3775 100644 --- a/flash.h +++ b/flash.h @@ -131,6 +131,7 @@ * other flash chips, such as the ENE KB9012 internal flash, work the opposite way. */ #define FEATURE_ERASED_ZERO (1 << 16) +#define FEATURE_NO_ERASE (1 << 17) #define ERASED_VALUE(flash) (((flash)->chip->feature_bits & FEATURE_ERASED_ZERO) ? 0x00 : 0xff) diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index 1600113..e732bcf 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -289,6 +289,8 @@ .sp .BR "* ogp_spi" " (for SPI flash ROMs on Open Graphics Project graphics card)" .sp +.BR "* linux_mtd" " (for SPI flash ROMs accessible via /dev/mtdX on Linux)" +.sp .BR "* linux_spi" " (for SPI flash ROMs accessible via /dev/spidevX.Y on Linux)" .sp .BR "* usbblaster_spi" " (for SPI flash ROMs attached to an Altera USB-Blaster compatible cable)" @@ -1007,6 +1009,19 @@ .B nic3com et al.\& section above. .SS +.BR "linux_mtd " programmer +.IP +You may specify the MTD device to use with the +.sp +.B " flashrom \-p linux_mtd:dev=/dev/mtdX" +.sp +syntax where +.B /dev/mtdX +is the Linux device node for your MTD device. If left unspecified the first MTD +device found (e.g. /dev/mtd0) will be used by default. +.sp +Please note that the linux_mtd driver only works on Linux. +.SS .BR "linux_spi " programmer .IP You have to specify the SPI controller to use with the diff --git a/flashrom.c b/flashrom.c index 776ad70..f85dbb1 100644 --- a/flashrom.c +++ b/flashrom.c @@ -341,6 +341,18 @@ }, #endif +#if CONFIG_LINUX_MTD == 1 + { + .name = "linux_mtd", + .type = OTHER, + .devs.note = "Device files /dev/mtd*\n", + .init = linux_mtd_init, + .map_flash_region = fallback_map, + .unmap_flash_region = fallback_unmap, + .delay = internal_delay, + }, +#endif + #if CONFIG_LINUX_SPI == 1 { .name = "linux_spi", @@ -1772,7 +1784,8 @@ bool skipped = true; uint8_t *const curcontents = info->curcontents + info->erase_start; const uint8_t erased_value = ERASED_VALUE(flashctx); - if (need_erase(curcontents, newcontents, erase_len, flashctx->chip->gran, erased_value)) { + if (!(flashctx->chip->feature_bits & FEATURE_NO_ERASE) && + need_erase(curcontents, newcontents, erase_len, flashctx->chip->gran, erased_value)) { if (erase_block(flashctx, info, erasefn)) goto _free_ret; /* Erase was successful. Adjust curcontents. */ diff --git a/internal.c b/internal.c index 9d2d8ad..1d6cff6 100644 --- a/internal.c +++ b/internal.c @@ -230,6 +230,9 @@ */ internal_buses_supported = BUS_NONSPI; + if (try_mtd() == 0) + return 0; + /* Initialize PCI access for flash enables */ if (pci_init_common() != 0) return 1; diff --git a/linux_mtd.c b/linux_mtd.c new file mode 100644 index 0000000..e65f093 --- /dev/null +++ b/linux_mtd.c @@ -0,0 +1,390 @@ +/* + * This file is part of the flashrom project. + * + * Copyright 2015 Google Inc. + * Copyright 2018-present Facebook, Inc. + * + * 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; version 2 of the License. + * + * 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. + */ + +#include <ctype.h> +#include <errno.h> +#include <fcntl.h> +#include <stdio.h> +#include <stdlib.h> +#include <mtd/mtd-user.h> +#include <string.h> +#include <sys/ioctl.h> +#include <sys/stat.h> +#include <unistd.h> + +#include "flash.h" +#include "programmer.h" + +#define LINUX_DEV_ROOT "/dev" +#define LINUX_MTD_SYSFS_ROOT "/sys/class/mtd" + +static FILE *dev_fp = NULL; + +static int mtd_device_is_writeable; + +static int mtd_no_erase; + +/* Size info is presented in bytes in sysfs. */ +static unsigned long int mtd_total_size; +static unsigned long int mtd_numeraseregions; +static unsigned long int mtd_erasesize; /* only valid if numeraseregions is 0 */ + +/* read a string from a sysfs file and sanitize it */ +static int read_sysfs_string(const char *sysfs_path, const char *filename, char *buf, int len) +{ + int i; + size_t bytes_read; + FILE *fp; + char path[strlen(LINUX_MTD_SYSFS_ROOT) + 32]; + + snprintf(path, sizeof(path), "%s/%s", sysfs_path, filename); + + if ((fp = fopen(path, "r")) == NULL) { + msg_perr("Cannot open %s\n", path); + return 1; + } + + clearerr(fp); + bytes_read = fread(buf, 1, (size_t)len, fp); + if (!feof(fp) && ferror(fp)) { + msg_perr("Error occurred when reading %s\n", path); + fclose(fp); + return 1; + } + + buf[bytes_read] = '\0'; + + /* + * Files from sysfs sometimes contain a newline or other garbage that + * can confuse functions like strtoul() and ruin formatting in print + * statements. Replace the first non-printable character (space is + * considered printable) with a proper string terminator. + */ + for (i = 0; i < len; i++) { + if (!isprint(buf[i])) { + buf[i] = '\0'; + break; + } + } + + fclose(fp); + return 0; +} + +static int read_sysfs_int(const char *sysfs_path, const char *filename, unsigned long int *val) +{ + char buf[32]; + char *endptr; + + if (read_sysfs_string(sysfs_path, filename, buf, sizeof(buf))) + return 1; + + errno = 0; + *val = strtoul(buf, &endptr, 0); + if (*endptr != '\0') { + msg_perr("Error reading %s\n", filename); + return 1; + } + + if (errno) { + msg_perr("Error reading %s: %s\n", filename, strerror(errno)); + return 1; + } + + return 0; +} + +static int popcnt(unsigned int u) +{ + int count = 0; + + while (u) { + u &= u - 1; + count++; + } + + return count; +} + +/* returns 0 to indicate success, non-zero to indicate error */ +static int get_mtd_info(const char *sysfs_path) +{ + unsigned long int tmp; + char mtd_device_name[32]; + + /* Flags */ + if (read_sysfs_int(sysfs_path, "flags", &tmp)) + return 1; + if (tmp & MTD_WRITEABLE) { + /* cache for later use by write function */ + mtd_device_is_writeable = 1; + } + if (tmp & MTD_NO_ERASE) { + mtd_no_erase = 1; + } + + /* Device name */ + if (read_sysfs_string(sysfs_path, "name", mtd_device_name, sizeof(mtd_device_name))) + return 1; + + /* Total size */ + if (read_sysfs_int(sysfs_path, "size", &mtd_total_size)) + return 1; + if (popcnt(mtd_total_size) != 1) { + msg_perr("MTD size is not a power of 2\n"); + return 1; + } + + /* Erase size */ + if (read_sysfs_int(sysfs_path, "erasesize", &mtd_erasesize)) + return 1; + if (popcnt(mtd_erasesize) != 1) { + msg_perr("MTD erase size is not a power of 2\n"); + return 1; + } + + /* Erase regions */ + if (read_sysfs_int(sysfs_path, "numeraseregions", &mtd_numeraseregions)) + return 1; + if (mtd_numeraseregions != 0) { + msg_perr("Non-uniform eraseblock size is unsupported.\n"); + return 1; + } + + msg_pdbg("%s: device_name: \"%s\", is_writeable: %d, " + "numeraseregions: %lu, total_size: %lu, erasesize: %lu\n", + __func__, mtd_device_name, mtd_device_is_writeable, + mtd_numeraseregions, mtd_total_size, mtd_erasesize); + + return 0; +} + +static int linux_mtd_probe(struct flashctx *flash) +{ + if (mtd_no_erase) + flash->chip->feature_bits |= FEATURE_NO_ERASE; + flash->chip->tested = TEST_OK_PREW; + flash->chip->total_size = mtd_total_size / 1024; /* bytes -> kB */ + flash->chip->block_erasers[0].eraseblocks[0].size = mtd_erasesize; + flash->chip->block_erasers[0].eraseblocks[0].count = mtd_total_size / mtd_erasesize; + return 1; +} + +static int linux_mtd_read(struct flashctx *flash, uint8_t *buf, + unsigned int start, unsigned int len) +{ + unsigned int eb_size = flash->chip->block_erasers[0].eraseblocks[0].size; + unsigned int i; + + if (fseek(dev_fp, start, SEEK_SET) != 0) { + msg_perr("Cannot seek to 0x%06x: %s\n", start, strerror(errno)); + return 1; + } + + for (i = 0; i < len; ) { + /* + * Try to align reads to eraseblock size. + * FIXME: Shouldn't actually be necessary, but not all MTD + * drivers handle arbitrary large reads well. + */ + unsigned int step = eb_size - ((start + i) % eb_size); + step = min(step, len - i); + + if (fread(buf + i, step, 1, dev_fp) != 1) { + msg_perr("Cannot read 0x%06x bytes at 0x%06x: %s\n", + step, start + i, strerror(errno)); + return 1; + } + + i += step; + } + + return 0; +} + +/* this version assumes we must divide the write request into chunks ourselves */ +static int linux_mtd_write(struct flashctx *flash, const uint8_t *buf, + unsigned int start, unsigned int len) +{ + unsigned int chunksize = flash->chip->block_erasers[0].eraseblocks[0].size; + unsigned int i; + + if (!mtd_device_is_writeable) + return 1; + + if (fseek(dev_fp, start, SEEK_SET) != 0) { + msg_perr("Cannot seek to 0x%06x: %s\n", start, strerror(errno)); + return 1; + } + + /* + * Try to align writes to eraseblock size. We want these large enough + * to give MTD room for optimizing performance. + * FIXME: Shouldn't need to divide this up at all, but not all MTD + * drivers handle arbitrary large writes well. + */ + for (i = 0; i < len; ) { + unsigned int step = chunksize - ((start + i) % chunksize); + step = min(step, len - i); + + if (fwrite(buf + i, step, 1, dev_fp) != 1) { + msg_perr("Cannot write 0x%06x bytes at 0x%06x\n", step, start + i); + return 1; + } + + if (fflush(dev_fp) == EOF) { + msg_perr("Failed to flush buffer: %s\n", strerror(errno)); + return 1; + } + + i += step; + } + + return 0; +} + +static int linux_mtd_erase(struct flashctx *flash, + unsigned int start, unsigned int len) +{ + uint32_t u; + + if (mtd_no_erase) { + msg_perr("%s: device does not support erasing. Please file a " + "bug report at flashrom(a)flashrom.org\n", __func__); + return 1; + } + + if (mtd_numeraseregions != 0) { + /* TODO: Support non-uniform eraseblock size using + use MEMGETREGIONCOUNT/MEMGETREGIONINFO ioctls */ + msg_perr("%s: mtd_numeraseregions must be 0\n", __func__); + return 1; + } + + for (u = 0; u < len; u += mtd_erasesize) { + struct erase_info_user erase_info = { + .start = start + u, + .length = mtd_erasesize, + }; + + if (ioctl(fileno(dev_fp), MEMERASE, &erase_info) == -1) { + msg_perr("%s: ioctl: %s\n", __func__, strerror(errno)); + return 1; + } + } + + return 0; +} + +static struct opaque_master programmer_linux_mtd = { + /* max_data_{read,write} don't have any effect for this programmer */ + .max_data_read = MAX_DATA_UNSPECIFIED, + .max_data_write = MAX_DATA_UNSPECIFIED, + .probe = linux_mtd_probe, + .read = linux_mtd_read, + .write = linux_mtd_write, + .erase = linux_mtd_erase, +}; + +/* Returns 0 if setup is successful, non-zero to indicate error */ +static int linux_mtd_setup(int dev_num) +{ + char sysfs_path[32]; + int ret = 1; + + /* Start by checking /sys/class/mtd/mtdN/type which should be "nor" for NOR flash */ + if (snprintf(sysfs_path, sizeof(sysfs_path), "%s/mtd%d/", LINUX_MTD_SYSFS_ROOT, dev_num) < 0) + goto linux_mtd_setup_exit; + + char buf[4]; + memset(buf, 0, sizeof(buf)); + if (read_sysfs_string(sysfs_path, "type", buf, sizeof(buf))) + return 1; + + if (strcmp(buf, "nor")) { + msg_perr("MTD device %d type is not \"nor\"\n", dev_num); + goto linux_mtd_setup_exit; + } + + /* sysfs shows the correct device type, see if corresponding device node exists */ + char dev_path[32]; + struct stat s; + snprintf(dev_path, sizeof(dev_path), "%s/mtd%d", LINUX_DEV_ROOT, dev_num); + errno = 0; + if (stat(dev_path, &s) < 0) { + msg_pdbg("Cannot stat \"%s\": %s\n", dev_path, strerror(errno)); + goto linux_mtd_setup_exit; + } + + /* so far so good, get more info from other files in this dir */ + if (snprintf(sysfs_path, sizeof(sysfs_path), "%s/mtd%d/", LINUX_MTD_SYSFS_ROOT, dev_num) < 0) + goto linux_mtd_setup_exit; + if (get_mtd_info(sysfs_path)) + goto linux_mtd_setup_exit; + + /* open file stream and go! */ + if ((dev_fp = fopen(dev_path, "r+")) == NULL) { + msg_perr("Cannot open file stream for %s\n", dev_path); + goto linux_mtd_setup_exit; + } + msg_pinfo("Opened %s successfully\n", dev_path); + + ret = 0; +linux_mtd_setup_exit: + return ret; +} + +static int linux_mtd_shutdown(void *data) +{ + if (dev_fp != NULL) { + fclose(dev_fp); + dev_fp = NULL; + } + + return 0; +} + +int linux_mtd_init(void) +{ + char *param; + int dev_num = 0; + int ret = 1; + + param = extract_programmer_param("dev"); + if (param) { + char *endptr; + + dev_num = strtol(param, &endptr, 0); + if ((*endptr != '\0') || (dev_num < 0)) { + msg_perr("Invalid device number %s. Use flashrom -p " + "linux_mtd:dev=N where N is a valid MTD\n" + "device number.\n", param); + goto linux_mtd_init_exit; + } + } + + if (linux_mtd_setup(dev_num)) + goto linux_mtd_init_exit; + + if (register_shutdown(linux_mtd_shutdown, NULL)) + goto linux_mtd_init_exit; + + register_opaque_master(&programmer_linux_mtd); + + ret = 0; +linux_mtd_init_exit: + return ret; +} diff --git a/programmer.h b/programmer.h index 5d9610a..e49f262 100644 --- a/programmer.h +++ b/programmer.h @@ -94,6 +94,9 @@ #if CONFIG_SATAMV == 1 PROGRAMMER_SATAMV, #endif +#if CONFIG_LINUX_MTD == 1 + PROGRAMMER_LINUX_MTD, +#endif #if CONFIG_LINUX_SPI == 1 PROGRAMMER_LINUX_SPI, #endif @@ -523,6 +526,11 @@ int buspirate_spi_init(void); #endif +/* linux_mtd.c */ +#if CONFIG_LINUX_MTD == 1 +int linux_mtd_init(void); +#endif + /* linux_spi.c */ #if CONFIG_LINUX_SPI == 1 int linux_spi_init(void); @@ -585,6 +593,9 @@ #if CONFIG_OGP_SPI == 1 || CONFIG_NICINTEL_SPI == 1 || CONFIG_RAYER_SPI == 1 || CONFIG_PONY_SPI == 1 || (CONFIG_INTERNAL == 1 && (defined(__i386__) || defined(__x86_64__))) SPI_CONTROLLER_BITBANG, #endif +#if CONFIG_LINUX_MTD == 1 + SPI_CONTROLLER_LINUX_MTD, +#endif #if CONFIG_LINUX_SPI == 1 SPI_CONTROLLER_LINUX, #endif @@ -678,6 +689,13 @@ void probe_superio_ite(void); int init_superio_ite(void); +#if CONFIG_LINUX_MTD == 1 +/* trivial wrapper to avoid cluttering internal_init() with #if */ +static inline int try_mtd(void) { return linux_mtd_init(); }; +#else +static inline int try_mtd(void) { return 1; }; +#endif + /* mcp6x_spi.c */ int mcp6x_spi_init(int want_spi); -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 10 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[master]: linux_mtd: Import driver from ChromiumOS
by Philipp Deppenwiese (Code Review) 17 May '18

17 May '18

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/25706 ) Change subject: linux_mtd: Import driver from ChromiumOS ...................................................................... Patch Set 9: Code-Review+2 -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 9 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 17 May 2018 15:45:54 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: linux_mtd: Import driver from ChromiumOS
by Nico Huber (Code Review) 17 May '18

17 May '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/25706 ) Change subject: linux_mtd: Import driver from ChromiumOS ...................................................................... Patch Set 9: Code-Review+2 -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 9 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 17 May 2018 08:31:37 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: linux_mtd: Import driver from ChromiumOS
by build bot (Jenkins) (Code Review) 17 May '18

17 May '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/25706 ) Change subject: linux_mtd: Import driver from ChromiumOS ...................................................................... Patch Set 9: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/1077/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/1298/ : SUCCESS -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 9 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 17 May 2018 07:55:02 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: linux_mtd: Import driver from ChromiumOS
by David Hendricks (Code Review) 17 May '18

17 May '18

David Hendricks has posted comments on this change. ( https://review.coreboot.org/25706 ) Change subject: linux_mtd: Import driver from ChromiumOS ...................................................................... Patch Set 9: Verified+1 (5 comments) Thanks! https://review.coreboot.org/#/c/25706/8//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/25706/8//COMMIT_MSG@7 PS8, Line 7: linux_mtd: Import driver from ChromiumOS > Paul asked to make this a statement and I generally agree. […] Sure. It doesn't matter in this case since the commit message is so short, but for future reference the limit for the summary line is 65 chars (https://www.coreboot.org/Git#Commit_messages) so don't expect perfect grammar all the time ;-) https://review.coreboot.org/#/c/25706/8//COMMIT_MSG@17 PS8, Line 17: it for other Linux interfaces later on. > Maybe also mention the switch to stream functions. Good call - Done https://review.coreboot.org/#/c/25706/8/flashrom.8.tmpl File flashrom.8.tmpl: https://review.coreboot.org/#/c/25706/8/flashrom.8.tmpl@1014 PS8, Line 1014: You may specify the MTD device to use with the > AIUI, this is optional (it would try to open /dev/mtd0 by default)? Done. I only implied that mtd0 will be the default since ideally we should let whatever auto-probe logic we add choose among whatever MTD devices/chips are in the system. https://review.coreboot.org/#/c/25706/8/linux_mtd.c File linux_mtd.c: https://review.coreboot.org/#/c/25706/8/linux_mtd.c@16 PS8, Line 16: : #include <ctype.h> : #include <errno.h> : #include <fcntl.h> : #include <stdio.h> : #include <stdlib.h> : #include <mtd/mtd-user.h> : #include <string.h> : #include <sys/ioctl.h> : #include <sys/stat.h> : #include <unistd.h> : > Not sure, if we (still) need all of these. I was able to get rid of a couple. https://review.coreboot.org/#/c/25706/8/linux_mtd.c@343 PS8, Line 343: msg_pinfo("Opened %s successfully\n", dev_path); Added an informational print. -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 9 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 17 May 2018 07:04:52 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: linux_mtd: Import driver from ChromiumOS
by David Hendricks (Code Review) 17 May '18

17 May '18

Hello Patrick Rudolph, Paul Menzel, Jonathan Neuschäfer, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Brian Norris, I'd like you to reexamine a change. Please visit https://review.coreboot.org/25706 to look at the new patch set (#9). Change subject: linux_mtd: Import driver from ChromiumOS ...................................................................... linux_mtd: Import driver from ChromiumOS This imports a series of patches from chromiumos for MTD support. The patches are squashed to ease review and original Change-Ids have been removed to avoid confusing Gerrit. There are a few changes to integrate the code: - Conflict resolution - Makefile changes - Remove file library usage from linux_mtd. We may revisit this and use it for other Linux interfaces later on. - Switch to using file stream functions for reads and writes. This consolidated patch is Signed-off-by: David Hendricks <dhendricks(a)fb.com> The first commit's message is: Initial MTD support This adds MTD support to flashrom so that we can read, erase, and write content on a NOR flash chip via MTD. BUG=chrome-os-partner:40208 BRANCH=none TEST=read, write, and erase works on Oak Signed-off-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/272983 Reviewed-by: Shawn N <shawnn(a)chromium.org> This is the 2nd commit message: linux_mtd: Fix compilation errors This fixes compilation errors from the initial import patch. Signed-off-by: David Hendricks <dhendricks(a)fb.com> This is the 3rd commit message: linux_mtd: Suppress message if NOR device not found This just suppresses a message that might cause confusion for unsuspecting users. BUG=none BRANCH=none TEST=ran on veyron_mickey, "NOR type device not found" message no longer appears under normal circumstances. Signed-off-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/302145 Commit-Ready: David Hendricks <dhendrix(a)chromium.org> Tested-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-by: Shawn N <shawnn(a)chromium.org> This is the 4th commit message: linux_mtd: Support for NO_ERASE type devices Some mtd devices have the MTD_NO_ERASE flag set. This means these devices don't require an erase to write and might not have implemented an erase function. We should be conservative and skip erasing altogether, falling back to performing writes over the whole flash. BUG=b:35104688 TESTED=Zaius flash is now written correctly for the 0xff regions. Signed-off-by: William A. Kennington III <wak(a)google.com> Reviewed-on: https://chromium-review.googlesource.com/472128 Commit-Ready: William Kennington <wak(a)google.com> Tested-by: William Kennington <wak(a)google.com> Reviewed-by: Brian Norris <briannorris(a)chromium.org> This is the 5th commit message: linux_mtd: do reads in eraseblock-sized chunks It's probably not the best idea to try to do an 8MB read in one syscall. Theoretically, this should work; but MTD just relies on the SPI driver to deliver the whole read in one transfer, and many SPI drivers haven't been tested well with large transfer sizes. I'd consider this a workaround, but it's still good to have IMO. BUG=chrome-os-partner:53215 TEST=boot kevin; `flashrom --read ...` TEST=check for performance regression on oak BRANCH=none Signed-off-by: Brian Norris <briannorris(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/344006 Reviewed-by: David Hendricks <dhendrix(a)chromium.org> This is the 6th commit message: linux_mtd: make read/write loop chunks consistent, and documented Theoretically, there should be no maximum size for the read() and write() syscalls on an MTD (well, except for the size of the entire device). But practical concerns (i.e., bugs) have meant we don't quite do this. For reads: Bug https://b/35573113 shows that some SPI-based MTD drivers don't yet handle very large transactions. So we artificially limit this to block-sized chunks. For writes: It's not clear there is a hard limit. Some drivers will already split large writes into smaller chunks automatically. Others don't do any splitting. At any rate, using *small* chunks can actually be a problem for some devices (b:35104688), as they get worse performance (doing an internal read/modify/write). This could be fixed in other ways by advertizing their true "write chunk size" to user space somehow, but this isn't so easy. As a simpler fix, we can just increase the loop increment to match the read loop. Per David, the original implementation (looping over page chunks) was just being paranoid. So this patch: * clarifies comments in linux_mtd_read(), to note that the chunking is somewhat of a hack that ideally can be fixed (with bug reference) * simplifies the linux_mtd_write() looping to match the structure in linux_mtd_read(), including dropping several unnecessary seeks, and correcting the error messages (they referred to "reads" and had the wrong parameters) * change linux_mtd_write() to align its chunks to eraseblocks, not page sizes Note that the "->page_size" parameter is still somewhat ill-defined, and only set by the upper layers for "opaque" flash. And it's not actually used in this driver now. If we could figure out what we really want to use it for, then we could try to set it appropriately. BRANCH=none BUG=b:35104688 TEST=various flashrom tests on Kevin TEST=Reading and writing to flash works on our zaius machines over mtd Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Signed-off-by: Brian Norris <briannorris(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/505409 Reviewed-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-by: Martin Roth <martinroth(a)chromium.org> Reviewed-by: William Kennington <wak(a)google.com> --- M Makefile M flash.h M flashrom.8.tmpl M flashrom.c M internal.c A linux_mtd.c M programmer.h 7 files changed, 475 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/25706/9 -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 9 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[master]: linux_mtd: Initial import
by build bot (Jenkins) (Code Review) 16 May '18

16 May '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/25706 ) Change subject: linux_mtd: Initial import ...................................................................... Patch Set 8: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/1076/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/1297/ : SUCCESS -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 8 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 16 May 2018 16:40:09 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: linux_mtd: Initial import
by Nico Huber (Code Review) 16 May '18

16 May '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/25706 ) Change subject: linux_mtd: Initial import ...................................................................... Patch Set 8: Code-Review+2 (5 comments) https://review.coreboot.org/#/c/25706/8//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/25706/8//COMMIT_MSG@7 PS8, Line 7: linux_mtd: Initial import Paul asked to make this a statement and I generally agree. How about "Import driver from CrOS"? https://review.coreboot.org/#/c/25706/8//COMMIT_MSG@17 PS8, Line 17: it for other Linux interfaces later on. Maybe also mention the switch to stream functions. https://review.coreboot.org/#/c/25706/8/flashrom.8.tmpl File flashrom.8.tmpl: PS8: There's also the automation for `-p internal`. https://review.coreboot.org/#/c/25706/8/flashrom.8.tmpl@1014 PS8, Line 1014: You have to specify the MTD device to use with the AIUI, this is optional (it would try to open /dev/mtd0 by default)? https://review.coreboot.org/#/c/25706/8/linux_mtd.c File linux_mtd.c: https://review.coreboot.org/#/c/25706/8/linux_mtd.c@16 PS8, Line 16: #include <ctype.h> : #include <errno.h> : #include <fcntl.h> : #include <inttypes.h> : #include <libgen.h> : #include <stdio.h> : #include <stdlib.h> : #include <mtd/mtd-user.h> : #include <string.h> : #include <sys/ioctl.h> : #include <sys/stat.h> : #include <unistd.h> Not sure, if we (still) need all of these. -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 8 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 16 May 2018 16:11:41 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: linux_mtd: Initial import
by David Hendricks (Code Review) 16 May '18

16 May '18

David Hendricks has posted comments on this change. ( https://review.coreboot.org/25706 ) Change subject: linux_mtd: Initial import ...................................................................... Patch Set 8: Verified+1 (1 comment) I looked into using fileno() as you had suggested and as it turns out it works pretty easily. For some reason I was thinking I'd need additional error handling, but it "should not fail" (according to the manpage). So with that assumption it fits in quite easily. https://review.coreboot.org/#/c/25706/7/linux_mtd.c File linux_mtd.c: https://review.coreboot.org/#/c/25706/7/linux_mtd.c@347 PS7, Line 347: return ret; > Missing error message and `close(dev_fd);`. Done -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 8 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 16 May 2018 15:56:07 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: linux_mtd: Initial import
by David Hendricks (Code Review) 16 May '18

16 May '18

Hello Patrick Rudolph, Paul Menzel, Jonathan Neuschäfer, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Brian Norris, I'd like you to reexamine a change. Please visit https://review.coreboot.org/25706 to look at the new patch set (#8). Change subject: linux_mtd: Initial import ...................................................................... linux_mtd: Initial import This imports a series of patches from chromiumos for MTD support. The patches are squashed to ease review and original Change-Ids have been removed to avoid confusing Gerrit. There are a few changes to integrate the code: - Conflict resolution - Makefile changes - Remove file library usage from linux_mtd. We may revisit this and use it for other Linux interfaces later on. This consolidated patch is Signed-off-by: David Hendricks <dhendricks(a)fb.com> The first commit's message is: Initial MTD support This adds MTD support to flashrom so that we can read, erase, and write content on a NOR flash chip via MTD. BUG=chrome-os-partner:40208 BRANCH=none TEST=read, write, and erase works on Oak Signed-off-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/272983 Reviewed-by: Shawn N <shawnn(a)chromium.org> This is the 2nd commit message: linux_mtd: Fix compilation errors This fixes compilation errors from the initial import patch. Signed-off-by: David Hendricks <dhendricks(a)fb.com> This is the 3rd commit message: linux_mtd: Suppress message if NOR device not found This just suppresses a message that might cause confusion for unsuspecting users. BUG=none BRANCH=none TEST=ran on veyron_mickey, "NOR type device not found" message no longer appears under normal circumstances. Signed-off-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/302145 Commit-Ready: David Hendricks <dhendrix(a)chromium.org> Tested-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-by: Shawn N <shawnn(a)chromium.org> This is the 4th commit message: linux_mtd: Support for NO_ERASE type devices Some mtd devices have the MTD_NO_ERASE flag set. This means these devices don't require an erase to write and might not have implemented an erase function. We should be conservative and skip erasing altogether, falling back to performing writes over the whole flash. BUG=b:35104688 TESTED=Zaius flash is now written correctly for the 0xff regions. Signed-off-by: William A. Kennington III <wak(a)google.com> Reviewed-on: https://chromium-review.googlesource.com/472128 Commit-Ready: William Kennington <wak(a)google.com> Tested-by: William Kennington <wak(a)google.com> Reviewed-by: Brian Norris <briannorris(a)chromium.org> This is the 5th commit message: linux_mtd: do reads in eraseblock-sized chunks It's probably not the best idea to try to do an 8MB read in one syscall. Theoretically, this should work; but MTD just relies on the SPI driver to deliver the whole read in one transfer, and many SPI drivers haven't been tested well with large transfer sizes. I'd consider this a workaround, but it's still good to have IMO. BUG=chrome-os-partner:53215 TEST=boot kevin; `flashrom --read ...` TEST=check for performance regression on oak BRANCH=none Signed-off-by: Brian Norris <briannorris(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/344006 Reviewed-by: David Hendricks <dhendrix(a)chromium.org> This is the 6th commit message: linux_mtd: make read/write loop chunks consistent, and documented Theoretically, there should be no maximum size for the read() and write() syscalls on an MTD (well, except for the size of the entire device). But practical concerns (i.e., bugs) have meant we don't quite do this. For reads: Bug https://b/35573113 shows that some SPI-based MTD drivers don't yet handle very large transactions. So we artificially limit this to block-sized chunks. For writes: It's not clear there is a hard limit. Some drivers will already split large writes into smaller chunks automatically. Others don't do any splitting. At any rate, using *small* chunks can actually be a problem for some devices (b:35104688), as they get worse performance (doing an internal read/modify/write). This could be fixed in other ways by advertizing their true "write chunk size" to user space somehow, but this isn't so easy. As a simpler fix, we can just increase the loop increment to match the read loop. Per David, the original implementation (looping over page chunks) was just being paranoid. So this patch: * clarifies comments in linux_mtd_read(), to note that the chunking is somewhat of a hack that ideally can be fixed (with bug reference) * simplifies the linux_mtd_write() looping to match the structure in linux_mtd_read(), including dropping several unnecessary seeks, and correcting the error messages (they referred to "reads" and had the wrong parameters) * change linux_mtd_write() to align its chunks to eraseblocks, not page sizes Note that the "->page_size" parameter is still somewhat ill-defined, and only set by the upper layers for "opaque" flash. And it's not actually used in this driver now. If we could figure out what we really want to use it for, then we could try to set it appropriately. BRANCH=none BUG=b:35104688 TEST=various flashrom tests on Kevin TEST=Reading and writing to flash works on our zaius machines over mtd Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Signed-off-by: Brian Norris <briannorris(a)chromium.org> Reviewed-on: https://chromium-review.googlesource.com/505409 Reviewed-by: David Hendricks <dhendrix(a)chromium.org> Reviewed-by: Martin Roth <martinroth(a)chromium.org> Reviewed-by: William Kennington <wak(a)google.com> --- M Makefile M flash.h M flashrom.8.tmpl M flashrom.c M internal.c A linux_mtd.c M programmer.h 7 files changed, 474 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/25706/8 -- To view, visit https://review.coreboot.org/25706 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3d6bb282863a5cf69909e28a1fc752b35f1b9599 Gerrit-Change-Number: 25706 Gerrit-PatchSet: 8 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit