Am 21.07.2011 17:54 schrieb Stefan Tauner:
> previously the dummies were initialized to be empty (all ones), which makes writes skip
> erasing altogether. with this patch the default is to fill it with random bytes instead and the
> old behavior can be enforced with stating "empty=yes" on the command line.
>
> Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
>
Does the following not work for you?
dd if=/dev/urandom bs=1k count=4k of=oldimage.bin
flashrom -p dummy:emulate=SST25VF032B,image=oldimage.bin -w newimage.bin
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
On Fri, 30 Aug 2013 02:00:34 +0200
Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at> wrote:
> This was mainly driven by getting rid of cli_classic_abort_usage() in cli_classic.c.
I forgot to mention something and give a rationale for it:
Thereby we also get rid of teasing the user to run flashrom again to
show usage information.
If we think the user should see the usage information we should show it
to him or at least a short syntax summary (which we pretty much lack
currently: the programmer parameter syntax is not really well
explained). (Small) GNU apps seem to favor printing a similar message
like we do. nmap, rsync give their whole usage information which is a
bit of a pain because it is so long (unlike flashrom's), ping and hd
are examples that just print a syntax summary (but they lack a complete
usage help). All of the above is stupid. :P Printing a wall of text
does not help and is even counterproductive if one can not scroll.
Printing how to print help is also a little bit silly if that help text
is not very long anyway. The right™ thing in the case of flashrom is
IMHO to print the complete usage help (after improving it to explain
the programmer parameter syntax better).
RFC.
>
> This makes the whole main function more consistent and fixes a myriad of
> (theoretical) memory leaks. This slightly changes output in most error cases.
> doit() stops calling programmer_shutdown() with this patch, beware.
--
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
On Sun, 24 Mar 2013 11:10:19 -0400
Justin Chevrier <jchevrier(a)gmail.com> wrote:
> From the 'You can never have enough programmers' file, comes support
> for another programmer type.
Supporting the internal one becomes more painful every day, so
expanding the support for external programmers is very welcomed, thanks!
> Specifically it's the PICkit2. This patch was inspired by the code in
> AVRDude (open source Atmel AVR
> programmer) to support the PICkit2 written by Doug Brown [1]. The
> Dediprog code in Flashrom was used
> as the template for this code with some reference to the ft2232 code
> as well. I left Carl-Daniel in
> the copyright assignment as I thought it made the most sense as I
> basically copied and pasted then
> modified the code as needed. Hopefully that was the right thing to do.
Sure, the copyright does not vanish when you copy some code to a new
file, so all previous copyright notices should be copied over too
(if one thinks they are useful at all... :)
> A few notes:
>
> 1) The code makes no provision for externally powered targets (where
> VDD from the PICkit can be connected to
> the device and an external power source and it will detect external
> power and not apply its own).
> I'm assuming that code needs to be written for this to work and that
> the PICkit itself doesn't make it happen
> automagically. Not sure on that one
I have *no* idea about that programmer or the protocol... but maybe
that's what the equivalent of voltage_selector = 0 is for?
>
> 2) I've tested with PICkit2 firmware version 2.32. I suspect that
> there are earlier versions of the
> firmware that are not capable of talking SPI, but I'm not sure. Should
> we include a warning if we detect
> that a user is running on older firmware and ask users to submit if
> their firmware version works
> for them?
Maybe... other programmers use their devices struct for such things. I
don't think dediprog is a good example in this regard.
> 3) I've only tested with the one easily accessible SPI chip I have on
> hand (GigaDevice GD25Q80)
>
> Connections are as follows:
>
> PICkit2 Pin 1 - VPP/MCLR -> CS
> PICkit2 Pin 2 - VDD -> Vcc
> PICkit2 Pin 3 - GND -> Vss
> PICkit2 Pin 4 - PGD -> SO
> PICkit2 Pin 5 - PDC -> SCLK
> PICkit2 Pin 6 - AUX -> SI
Creating a wiki page would be welcomed. If you want an account to do
that, just ask.
> Feedback welcomed! Hopefully this is useful to someone.
>
> Thanks for the great tools as always guys.
Thanks for your patch!
The following is not a full review, but I mention a few things I
spotted while skimming through. Feel free to ignore them (for now :) if
you don't agree.
> Index: Makefile
> ===================================================================
> --- Makefile (revision 1657)
> +++ Makefile (working copy)
> @@ -117,7 +117,7 @@
> else
> override CONFIG_PONY_SPI = no
> endif
> -# Dediprog and FT2232 are not supported under DOS (missing USB support).
> +# Dediprog, FT2232 and PICkit2 are not supported under DOS (missing USB support).
> ifeq ($(CONFIG_DEDIPROG), yes)
> UNSUPPORTED_FEATURES += CONFIG_DEDIPROG=yes
> else
> @@ -128,7 +128,12 @@
> else
> override CONFIG_FT2232_SPI = no
> endif
> +ifeq ($(CONFIG_PICKIT2_SPI), yes)
> +UNSUPPORTED_FEATURES += CONFIG_PICKIT2_SPI=yes
> +else
> +override CONFIG_PICKIT2_SPI = no
> endif
> +endif
>
> # FIXME: Should we check for Cygwin/MSVC as well?
> ifeq ($(TARGET_OS), MinGW)
> @@ -229,7 +234,7 @@
> else
> override CONFIG_SERPROG = no
> endif
> -# Dediprog and FT2232 are not supported with libpayload (missing libusb support)
> +# Dediprog, FT2232 and PICkit2 are not supported with libpayload (missing libusb support)
> ifeq ($(CONFIG_DEDIPROG), yes)
> UNSUPPORTED_FEATURES += CONFIG_DEDIPROG=yes
> else
> @@ -240,7 +245,12 @@
> else
> override CONFIG_FT2232_SPI = no
> endif
> +ifeq ($(CONFIG_PICKIT2_SPI), yes)
> +UNSUPPORTED_FEATURES += CONFIG_PICKIT2_SPI=yes
> +else
> +override CONFIG_PICKIT2_SPI = no
> endif
> +endif
>
> ifneq ($(TARGET_OS), Linux)
> ifeq ($(CONFIG_LINUX_SPI), yes)
> @@ -382,6 +392,9 @@
> # Enable Linux spidev interface by default. We disable it on non-Linux targets.
> CONFIG_LINUX_SPI ?= yes
>
> +# Always enable PICkit2 SPI dongles for now.
> +CONFIG_PICKIT2_SPI ?= yes
> +
> # Disable wiki printing by default. It is only useful if you have wiki access.
> CONFIG_PRINT_WIKI ?= no
>
> @@ -549,6 +562,12 @@
> PROGRAMMER_OBJS += linux_spi.o
> endif
>
> +ifeq ($(CONFIG_PICKIT2_SPI), yes)
> +FEATURE_CFLAGS += -D'CONFIG_PICKIT2_SPI=1'
> +PROGRAMMER_OBJS += pickit2_spi.o
> +NEED_USB := yes
> +endif
> +
> ifeq ($(NEED_SERIAL), yes)
> LIB_OBJS += serial.o
> endif
> Index: flashrom.8
> ===================================================================
> --- flashrom.8 (revision 1657)
> +++ flashrom.8 (working copy)
> @@ -220,6 +220,8 @@
> .sp
> .BR "* linux_spi" " (for SPI flash ROMs accessible via /dev/spidevX.Y on Linux)"
> .sp
> +.BR "* pickit2_spi" " (for SPI flash ROMs accessible via Microchip PICkit2 on Linux)"
Linux only? I guess that's a copy & paste error and it will work whereever flashrom and libusb work.
> +.sp
> Some programmers have optional or mandatory parameters which are described
> in detail in the
> .B PROGRAMMER SPECIFIC INFO
TODO: explanation of programmer parameters in an detailed section.
> Index: flashrom.c
> ===================================================================
> --- flashrom.c (revision 1657)
> +++ flashrom.c (working copy)
> @@ -308,6 +308,19 @@
> },
> #endif
>
> +#if CONFIG_PICKIT2_SPI == 1
> + {
> + .name = "pickit2_spi",
> + .type = OTHER,
> + /* FIXME */
> + .devs.note = "Microchip PICkit2\n",
> + .init = pickit2_spi_init,
> + .map_flash_region = fallback_map,
> + .unmap_flash_region = fallback_unmap,
> + .delay = internal_delay,
> + },
> +#endif
> +
> {0}, /* This entry corresponds to PROGRAMMER_INVALID. */
> };
>
> Index: pickit2_spi.c
> ===================================================================
> --- pickit2_spi.c (revision 0)
> +++ pickit2_spi.c (working copy)
> @@ -0,0 +1,519 @@
> +/*
> + * This file is part of the flashrom project.
> + *
> + * Copyright (C) 2010 Carl-Daniel Hailfinger
> + * Copyright (C) 2013 Justin Chevrier
> + *
> + * 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.
> + *
> + * 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
> + */
> +
Links to or citations of relevant documentation should go here (or the
to be written manpage section, if useful to users too), at least a
reference to avrdude should be given IMHO, but feel free to be as
verbose as the commit log.
> +#include <stdio.h>
> +#include <string.h>
> +#include <limits.h>
> +#include <errno.h>
> +#include <usb.h>
> +#include "flash.h"
> +#include "chipdrivers.h"
> +#include "programmer.h"
> +#include "spi.h"
> +
> +static usb_dev_handle *pickit2_handle;
> +
> +#define DEFAULT_TIMEOUT 10000
which unit? when is it used? etc.
> +
> +#define CMD_LENGTH 64
> +#define ENDPOINT_OUT 0x01
> +#define ENDPOINT_IN 0x81
> +
> +#define PICKIT2_VID 0x04D8
> +#define PICKIT2_PID 0x0033
> +
> +#define CMD_GET_VERSION 0x76
> +#define CMD_SET_VDD 0xA0
> +#define CMD_SET_VPP 0xA1
> +#define CMD_READ_VDD_VPP 0xA3
> +#define CMD_EXEC_SCRIPT 0xA6
> +#define CMD_CLR_DLOAD_BUFF 0xA7
> +#define CMD_DOWNLOAD_DATA 0xA8
> +#define CMD_CLR_ULOAD_BUFF 0xA9
> +#define CMD_UPLOAD_DATA 0xAA
> +#define CMD_END_OF_BUFFER 0xAD
> +
> +#define SCR_SPI_READ_BUF 0xC5
> +#define SCR_SPI_WRITE_BUF 0xC6
> +#define SCR_SET_AUX 0xCF
> +#define SCR_LOOP 0xE9
> +#define SCR_SET_ICSP_CLK_PERIOD 0xEA
> +#define SCR_SET_PINS 0xF3
> +#define SCR_BUSY_LED_OFF 0xF4
> +#define SCR_BUSY_LED_ON 0xF5
> +#define SCR_MCLR_GND_OFF 0xF6
> +#define SCR_MCLR_GND_ON 0xF7
> +#define SCR_VPP_PWM_OFF 0xF8
> +#define SCR_VPP_PWM_ON 0xF9
> +#define SCR_VPP_OFF 0xFA
> +#define SCR_VPP_ON 0xFB
> +#define SCR_VDD_OFF 0xFE
> +#define SCR_VDD_ON 0xFF
> +
> +/* Might be useful for other USB devices as well. static for now. */
We need to sort stuff like this out, but that's not your problem.
> +/* device parameter allows user to specify one device of multiple installed */
> +static struct usb_device *get_device_by_vid_pid(uint16_t vid, uint16_t pid, unsigned int device)
> +{
> + struct usb_bus *bus;
> + struct usb_device *dev;
> +
> + for (bus = usb_get_busses(); bus; bus = bus->next)
> + for (dev = bus->devices; dev; dev = dev->next)
> + if ((dev->descriptor.idVendor == vid) &&
> + (dev->descriptor.idProduct == pid)) {
> + if (device == 0)
> + return dev;
> + device--;
> + }
> +
> + return NULL;
> +}
> +
> +static int pickit2_get_firmware_version(void)
> +{
> + int ret;
> + unsigned char command[CMD_LENGTH] = {CMD_GET_VERSION, CMD_END_OF_BUFFER};
> +
> + ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DEFAULT_TIMEOUT);
> + ret = usb_interrupt_read(pickit2_handle, ENDPOINT_IN, (char *)command, CMD_LENGTH, DEFAULT_TIMEOUT);
> +
> + msg_pdbg("PICkit2 Firmware Version: %d.%d\n", (int)command[0], (int)command[1]);
> + if (ret != CMD_LENGTH) {
> + msg_perr("Command Get Firmware Version failed (%s)!\n",
> + usb_strerror());
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static int pickit2_set_spi_voltage(int millivolt)
> +{
> + int ret;
> + uint16_t voltage_selector;
> […]
> + voltage_selector = 1.8;
that would be in the best case some "creative" usage of floating point constants. ;)
> […]
> +
> + unsigned char command[CMD_LENGTH] = {
I personally favor uint8_t instead of unsigned chars...
> + CMD_SET_VDD,
> + (uint8_t)(voltage_selector * 2048+672),
... especially if you cast to them anyway :)
> + (uint8_t)((voltage_selector * 2048+672)/256),
We normally add spaces around all binary ops.
> + (uint8_t)(voltage_selector * 36),
> + CMD_SET_VPP,
> + 0x40,
> + (uint8_t)(voltage_selector * 18.61),
I am not sure I want to understand that protocol better, if this is correct. :)
> + (uint8_t)(voltage_selector * 13),
> + CMD_END_OF_BUFFER
> + };
> +
> + ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DEFAULT_TIMEOUT);
> +
> + if (ret != CMD_LENGTH) {
> + msg_perr("Command Set Voltage failed (%s)!\n",
> + usb_strerror());
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +
> +struct pickit2_spispeeds {
> + const char *const name;
> + const int speed;
> +};
> +
> +static const struct pickit2_spispeeds spispeeds[] = {
> + { "1M", 0x1 },
> + { "500k", 0x2 },
> + { "333k", 0x3 },
> + { "250k", 0x4 },
> + { NULL, 0x0 },
> +};
> +
> +static int pickit2_set_spi_speed(unsigned int spispeed_idx)
> +{
> + int ret;
> +
> + msg_pdbg("SPI speed is %sHz\n", spispeeds[spispeed_idx].name);
> +
> + unsigned char command[CMD_LENGTH] = {
> + CMD_EXEC_SCRIPT,
> + 2,
> + SCR_SET_ICSP_CLK_PERIOD,
> + spispeed_idx,
> + CMD_END_OF_BUFFER
> + };
> +
> + ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DEFAULT_TIMEOUT);
> +
> + if (ret != CMD_LENGTH) {
> + msg_perr("Command Set SPI Speed failed (%s)!\n",
> + usb_strerror());
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static int pickit2_spi_send_command(struct flashctx *flash,
> + unsigned int writecnt,
> + unsigned int readcnt,
> + const unsigned char *writearr,
> + unsigned char *readarr)
> +{
> + int ret,i;
> +
> + unsigned char buf[CMD_LENGTH] = {CMD_DOWNLOAD_DATA, writecnt};
> +
> + /* Maximum number of bytes per transaction (including command overhead) is 64. Lets play it safe
> + * and always assume the worst case scenario of 20 bytes command overhead.
> + */
> + msg_pspew("%s, writecnt=%i, readcnt=%i\n", __func__, writecnt, readcnt);
> + if (writecnt + readcnt + 20 > CMD_LENGTH) {
> + msg_perr("\nTotal packetsize (%i) is greater than 64 supported, aborting.\n", writecnt + readcnt + 20);
> + return 1;
> + }
> +
> + for (i = 2; i < writecnt + 2; i++)
> + {
The curly brace should be at the end of the "for line" according to our style.
> + buf[i] = writearr[i - 2];
> + }
> +
> + buf[i++] = CMD_CLR_ULOAD_BUFF;
> + buf[i++] = CMD_EXEC_SCRIPT;
> +
> + /* Determine script length based on number of bytes to be read or written */
> + if (writecnt == 1 && readcnt == 1)
> + buf[i++] = 7;
> + else if (writecnt == 1 || readcnt == 1)
> + buf[i++] = 10;
> + else
> + buf[i++] = 13;
> +
> + /* Assert CS# */
> + buf[i++] = SCR_VPP_OFF;
> + buf[i++] = SCR_MCLR_GND_ON;
> +
> + buf[i++] = SCR_SPI_WRITE_BUF;
> +
> + if (writecnt > 1) {
> + buf[i++] = SCR_LOOP;
> + buf[i++] = 1; /* Loop back one instruction */
> + buf[i++] = writecnt - 1; /* Number of times to loop */
> + }
> +
> + if (readcnt)
> + buf[i++] = SCR_SPI_READ_BUF;
> +
> + if (readcnt > 1) {
> + buf[i++] = SCR_LOOP;
> + buf[i++] = 1; /* Loop back one instruction */
> + buf[i++] = readcnt - 1; /* Number of times to loop */
> + }
> +
> + /* De-assert CS# */
> + buf[i++] = SCR_MCLR_GND_OFF;
> + buf[i++] = SCR_VPP_PWM_ON;
> + buf[i++] = SCR_VPP_ON;
> +
> + buf[i++] = CMD_UPLOAD_DATA;
> + buf[i++] = CMD_END_OF_BUFFER;
> +
> + ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)buf, CMD_LENGTH, DEFAULT_TIMEOUT);
> +
> + if (ret != CMD_LENGTH) {
> + msg_perr("Send SPI failed, expected %i, got %i %s!\n",
> + writecnt, ret, usb_strerror());
> + return 1;
> + }
> +
> + if (readcnt) {
> + memset(readarr, 0, readcnt);
> + memset(buf, 0, sizeof(buf));
Is this really necessary?
> +
> + ret = usb_interrupt_read(pickit2_handle, ENDPOINT_IN, (char *)buf, CMD_LENGTH, DEFAULT_TIMEOUT);
> +
> + if (ret != CMD_LENGTH) {
> + msg_perr("Receive SPI failed, expected %i, got %i %s!\n",
> + readcnt, ret, usb_strerror());
> + return 1;
> + }
> +
> + /* First byte indicates number of bytes transferred from upload buffer */
> + if (buf[0] != readcnt) {
> + msg_perr("Unexpected number of bytes transferred, expected %i, got %i!\n",
> + readcnt, ret);
> + return 1;
> + }
> +
> + /* Actual data starts at byte number two */
> + memcpy(readarr, &buf[1], readcnt);
> + }
> +
> + return 0;
> +}
> +
This is a candidate for a ultity "class" too, please add the "might be useful" comment here too:
/* Might be useful for other USB devices as well. static for now. */
And maybe add a reference to dediprog (and likewise for other copied
functions) so that we know immediately that they have been copied.
> +static int parse_voltage(char *voltage)
> +{
> + char *tmp = NULL;
> + int i;
> + int millivolt = 0, fraction = 0;
> +
> + if (!voltage || !strlen(voltage)) {
> + msg_perr("Empty voltage= specified.\n");
> + return -1;
> + }
> + millivolt = (int)strtol(voltage, &tmp, 0);
> + voltage = tmp;
> + /* Handle "," and "." as decimal point. Everything after it is assumed
> + * to be in decimal notation.
> + */
> + if ((*voltage == '.') || (*voltage == ',')) {
> + voltage++;
> + for (i = 0; i < 3; i++) {
> + fraction *= 10;
> + /* Don't advance if the current character is invalid,
> + * but continue multiplying.
> + */
> + if ((*voltage < '0') || (*voltage > '9'))
> + continue;
> + fraction += *voltage - '0';
> + voltage++;
> + }
> + /* Throw away remaining digits. */
> + voltage += strspn(voltage, "0123456789");
> + }
> + /* The remaining string must be empty or "mV" or "V". */
> + tolower_string(voltage);
> +
> + /* No unit or "V". */
> + if ((*voltage == '\0') || !strncmp(voltage, "v", 1)) {
> + millivolt *= 1000;
> + millivolt += fraction;
> + } else if (!strncmp(voltage, "mv", 2) ||
> + !strncmp(voltage, "milliv", 6)) {
> + /* No adjustment. fraction is discarded. */
> + } else {
> + /* Garbage at the end of the string. */
> + msg_perr("Garbage voltage= specified.\n");
> + return -1;
> + }
> + return millivolt;
> +}
> +
I think this function can be inlined without disadvantages. The comment
is clear enough.
OTOH it might be worth it if you can generify a pickit2_teardown or
similar (the code in the shutdown function).
> +static int pickit2_setup(void)
> +{
> + int ret;
> + /* Configure pins for correct directions and logic levels,
> + * turn Vdd on, turn busy LED on and clear buffers
> + */
> + unsigned char buf[CMD_LENGTH] = {
> + CMD_EXEC_SCRIPT,
> + 10, /* Script length */
> + SCR_SET_PINS,
> + /* Bit-0=0(PDC Out), Bit-1=1(PGD In), Bit-2=0(PDC LL), Bit-3=0(PGD LL) */
> + 2,
> + SCR_SET_AUX,
> + /* Bit-0=0(Aux Out), Bit-1=0(Aux LL) */
> + 0,
> + SCR_VDD_ON,
> + SCR_MCLR_GND_OFF, /* Let CS# float */
> + SCR_VPP_PWM_ON,
> + SCR_VPP_ON, /* Pull CS# high */
> + SCR_BUSY_LED_ON,
> + CMD_CLR_DLOAD_BUFF,
> + CMD_CLR_ULOAD_BUFF,
> + CMD_END_OF_BUFFER
> + };
> +
> + ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)buf, CMD_LENGTH, DEFAULT_TIMEOUT);
> +
> + if (ret != CMD_LENGTH) {
> + msg_perr("Command Setup failed (%s)!\n",
> + usb_strerror());
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +[…]
> +
> +static int pickit2_shutdown(void *data)
> +{
> + int ret;
> + msg_pspew("%s\n", __func__);
> +
> + /* Set all pins to float and turn voltages off */
> + unsigned char command[CMD_LENGTH] = {
> + CMD_EXEC_SCRIPT,
> + 8,
> + SCR_SET_PINS,
> + /* Bit-0=1(PDC In), Bit-1=1(PGD In), Bit-2=0(PDC LL), Bit-3=0(PGD LL) */
> + 3,
> + SCR_SET_AUX,
> + /* Bit-0=1(Aux In), Bit-1=0(Aux LL) */
> + 1,
> + SCR_MCLR_GND_OFF,
> + SCR_VPP_OFF,
> + SCR_VDD_OFF,
> + SCR_BUSY_LED_OFF,
> + CMD_END_OF_BUFFER
> + };
> +
> + ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DEFAULT_TIMEOUT);
> +
> + if (ret != CMD_LENGTH) {
> + msg_perr("Command Shutdown failed (%s)!\n",
> + usb_strerror());
> + return 1;
> + }
> +
> + if (usb_release_interface(pickit2_handle, 0)) {
> + msg_perr("Could not release USB interface!\n");
> + return 1;
> + }
> + if (usb_close(pickit2_handle)) {
> + msg_perr("Could not close USB device!\n");
> + return 1;
> + }
> + return 0;
> […]
--
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
I've tried to run flashrom -V to see if it could identify the chip on my
new board (AsRock M3A-GLAN), and it got stuck while probing for the
first chip.
I've tried commenting out that specific chip (AMIC A25L05PT) and then it
gets stuck on the next one. Then commented out all AMIC chips, and it
got stuck on the next one (the first ATMEL on the list)
I've tried with an older flashrom (0.9.2-r1141). It skipped some AMD
chips, then got stuck on AMIC again.
Talked with "tlt" and "stefanct" on irc and they asked for a strace and
a gdb backtrace. Strace produced no results. Gdb log is attached.
"stefanct" tried and reproduced it on a machine with nicintel_spi, but
warned it was with an unsupported nic:
http://paste.flashrom.org/view.php?id=738
--
GatoLoko
spi_read_status_register() is used in open-coded loops everywhere just
to check if SPI_SR_WIP bit cleared. The logic is missing a timeout
detection, and we can save quite a lot of code by introducing a function
which waits until SPI_SR_WIP is cleared or a timeout is reached.
Untested. May explode.
Unfinished. More functions need to be converted to the new style.
Will change behaviour if the chip is too slow or hangs. Should prevent
flashrom from hanging without any visible output.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Index: flashrom-spi_rdsr_refactor/spi25.c
===================================================================
--- flashrom-spi_rdsr_refactor/spi25.c (Revision 1653)
+++ flashrom-spi_rdsr_refactor/spi25.c (Arbeitskopie)
@@ -328,14 +328,9 @@
__func__);
return result;
}
- /* Wait until the Write-In-Progress bit is cleared.
- * This usually takes 1-85 s, so wait in 1 s steps.
- */
- /* FIXME: We assume spi_read_status_register will never fail. */
- while (spi_read_status_register(flash) & SPI_SR_WIP)
- programmer_delay(1000 * 1000);
/* FIXME: Check the status register for errors. */
- return 0;
+ result = spi_wait_status_register_ready(flash, 120 * 1000 * 1000, JEDEC_CE_60);
+ return result;
}
int spi_chip_erase_62(struct flashctx *flash)
@@ -365,14 +360,9 @@
__func__);
return result;
}
- /* Wait until the Write-In-Progress bit is cleared.
- * This usually takes 2-5 s, so wait in 100 ms steps.
- */
- /* FIXME: We assume spi_read_status_register will never fail. */
- while (spi_read_status_register(flash) & SPI_SR_WIP)
- programmer_delay(100 * 1000);
/* FIXME: Check the status register for errors. */
- return 0;
+ result = spi_wait_status_register_ready(flash, 120 * 1000 * 1000, JEDEC_CE_62);
+ return result;
}
int spi_chip_erase_c7(struct flashctx *flash)
@@ -401,14 +391,9 @@
msg_cerr("%s failed during command execution\n", __func__);
return result;
}
- /* Wait until the Write-In-Progress bit is cleared.
- * This usually takes 1-85 s, so wait in 1 s steps.
- */
- /* FIXME: We assume spi_read_status_register will never fail. */
- while (spi_read_status_register(flash) & SPI_SR_WIP)
- programmer_delay(1000 * 1000);
/* FIXME: Check the status register for errors. */
- return 0;
+ result = spi_wait_status_register_ready(flash, 120 * 1000 * 1000, JEDEC_CE_C7);
+ return result;
}
int spi_block_erase_52(struct flashctx *flash, unsigned int addr,
@@ -444,13 +429,9 @@
__func__, addr);
return result;
}
- /* Wait until the Write-In-Progress bit is cleared.
- * This usually takes 100-4000 ms, so wait in 100 ms steps.
- */
- while (spi_read_status_register(flash) & SPI_SR_WIP)
- programmer_delay(100 * 1000);
/* FIXME: Check the status register for errors. */
- return 0;
+ result = spi_wait_status_register_ready(flash, 10 * 1000 * 1000, JEDEC_BE_52);
+ return result;
}
/* Block size is usually
@@ -491,13 +472,9 @@
__func__, addr);
return result;
}
- /* Wait until the Write-In-Progress bit is cleared.
- * This usually takes 100-4000 ms, so wait in 100 ms steps.
- */
- while (spi_read_status_register(flash) & SPI_SR_WIP)
- programmer_delay(100 * 1000);
/* FIXME: Check the status register for errors. */
- return 0;
+ result = spi_wait_status_register_ready(flash, 10 * 1000 * 1000, JEDEC_BE_D8);
+ return result;
}
/* Block size is usually
@@ -536,13 +513,9 @@
__func__, addr);
return result;
}
- /* Wait until the Write-In-Progress bit is cleared.
- * This usually takes 100-4000 ms, so wait in 100 ms steps.
- */
- while (spi_read_status_register(flash) & SPI_SR_WIP)
- programmer_delay(100 * 1000);
/* FIXME: Check the status register for errors. */
- return 0;
+ result = spi_wait_status_register_ready(flash, 10 * 1000 * 1000, JEDEC_BE_D7);
+ return result;
}
/* Sector size is usually 4k, though Macronix eliteflash has 64k */
@@ -579,13 +552,9 @@
__func__, addr);
return result;
}
- /* Wait until the Write-In-Progress bit is cleared.
- * This usually takes 15-800 ms, so wait in 10 ms steps.
- */
- while (spi_read_status_register(flash) & SPI_SR_WIP)
- programmer_delay(10 * 1000);
/* FIXME: Check the status register for errors. */
- return 0;
+ result = spi_wait_status_register_ready(flash, 10 * 1000 * 1000, JEDEC_SE);
+ return result;
}
int spi_block_erase_50(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
@@ -619,13 +588,9 @@
msg_cerr("%s failed during command execution at address 0x%x\n", __func__, addr);
return result;
}
- /* Wait until the Write-In-Progress bit is cleared.
- * This usually takes 10 ms, so wait in 1 ms steps.
- */
- while (spi_read_status_register(flash) & SPI_SR_WIP)
- programmer_delay(1 * 1000);
/* FIXME: Check the status register for errors. */
- return 0;
+ result = spi_wait_status_register_ready(flash, 10 * 1000 * 1000, JEDEC_BE_50);
+ return result;
}
int spi_block_erase_81(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
@@ -659,13 +624,9 @@
msg_cerr("%s failed during command execution at address 0x%x\n", __func__, addr);
return result;
}
- /* Wait until the Write-In-Progress bit is cleared.
- * This usually takes 8 ms, so wait in 1 ms steps.
- */
- while (spi_read_status_register(flash) & SPI_SR_WIP)
- programmer_delay(1 * 1000);
/* FIXME: Check the status register for errors. */
- return 0;
+ result = spi_wait_status_register_ready(flash, 10 * 1000 * 1000, JEDEC_BE_81);
+ return result;
}
int spi_block_erase_60(struct flashctx *flash, unsigned int addr,
Index: flashrom-spi_rdsr_refactor/spi25_statusreg.c
===================================================================
--- flashrom-spi_rdsr_refactor/spi25_statusreg.c (Revision 1653)
+++ flashrom-spi_rdsr_refactor/spi25_statusreg.c (Arbeitskopie)
@@ -43,7 +43,6 @@
static int spi_write_status_register_flag(struct flashctx *flash, int status, const unsigned char enable_opcode)
{
int result;
- int i = 0;
/*
* WRSR requires either EWSR or WREN depending on chip type.
* The code below relies on the fact hat EWSR and WREN have the same
@@ -78,17 +77,10 @@
/* WRSR performs a self-timed erase before the changes take effect.
* This may take 50-85 ms in most cases, and some chips apparently
* allow running RDSR only once. Therefore pick an initial delay of
- * 100 ms, then wait in 10 ms steps until a total of 5 s have elapsed.
+ * 100 ms, then wait until a total of 5 s have elapsed.
*/
programmer_delay(100 * 1000);
- while (spi_read_status_register(flash) & SPI_SR_WIP) {
- if (++i > 490) {
- msg_cerr("Error: WIP bit after WRSR never cleared\n");
- return TIMEOUT_ERROR;
- }
- programmer_delay(10 * 1000);
- }
- return 0;
+ return spi_wait_status_register_ready(flash, 4900 * 1000, JEDEC_WRSR);
}
int spi_write_status_register(struct flashctx *flash, int status)
@@ -108,6 +100,28 @@
return ret;
}
+/* timeout is specified in usecs. */
+int spi_wait_status_register_ready(struct flashctx *flash, int timeout, uint8_t opcode)
+{
+ /* At least 1 usec between iterations. */
+ int single_delay = (timeout / 100) ? : 1;
+ int elapsed = 0;
+ int halftime = 0;
+
+ while (spi_read_status_register(flash) & SPI_SR_WIP) {
+ elapsed += single_delay;
+ if ((elapsed > timeout / 2) && !halftime) {
+ msg_cdbg("Debug: WIP bit after %02x didn't clear within %i us.\n", opcode, timeout/2);
+ }
+ if (elapsed >= timeout) {
+ msg_cerr("Error: WIP bit after %02x didn't clear within %i us.\n", opcode, timeout);
+ return TIMEOUT_ERROR;
+ }
+ programmer_delay(single_delay);
+ }
+ return 0;
+}
+
uint8_t spi_read_status_register(struct flashctx *flash)
{
static const unsigned char cmd[JEDEC_RDSR_OUTSIZE] = { JEDEC_RDSR };
Index: flashrom-spi_rdsr_refactor/chipdrivers.h
===================================================================
--- flashrom-spi_rdsr_refactor/chipdrivers.h (Revision 1653)
+++ flashrom-spi_rdsr_refactor/chipdrivers.h (Arbeitskopie)
@@ -60,6 +60,7 @@
/* spi25_statusreg.c */
uint8_t spi_read_status_register(struct flashctx *flash);
+int spi_wait_status_register_ready(struct flashctx *flash, int timeout, uint8_t opcode);
int spi_write_status_register(struct flashctx *flash, int status);
int spi_prettyprint_status_register_plain(struct flashctx *flash);
int spi_prettyprint_status_register_default_bp1(struct flashctx *flash);
--
http://www.hailfinger.org/
Hi
I came across two problems using 2 MiB FWH flash on two different boards
with old ICH4 and ICH6 chipsets:
First, ICH3/4/5 do not implement fwh_idsel= parameter. If vendor bios
had the default map of 1 MiB, it did not even detect 2 MiB flash chips.
For this I have patches prepared already.
Before: (idsel_failure.txt)
Probing for SST SST49LF008C, 1024 kB: probe_82802ab: id1 0xbf, id2 0x5c
Probing for SST SST49LF016C, 2048 kB: probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content
After: (idsel_ok.txt)
Probing for SST SST49LF008C, 1024 kB: probe_82802ab: id1 0xbf, id2 0x5c
Probing for SST SST49LF016C, 2048 kB: probe_82802ab: id1 0xbf, id2 0x5c
Found SST flash chip "SST49LF016C" (2048 kB, FWH) at physical address 0xffe00000.
Second, a board with ICH6 had all except one 512 kiB range of flash
disabled by vendor bios and it would not detect 1MiB and 2MiB flash
chips. NOTE: the board actually had 512kiB flash for these logs, but you
can see where ther problem is.
Before: (decode-disabled.txt)
0xfff80000/0xffb80000 FWH decode enabled
0xfff00000/0xffb00000 FWH decode disabled
0xffe80000/0xffa80000 FWH decode disabled
0xffe00000/0xffa00000 FWH decode disabled
...
Maximum FWH chip size: 0x80000 bytes
...
Probing for SST SST49LF008C, 1024 kB: Chip size 1024 kB is bigger than supported size 512 kB of chipset/board/programmer for FWH interface, probe/read/erase/write may fail. probe_82802ab: id1 0xff, id2 0xff, id1 parity violation, id1 is normal flash content, id2 is normal flash content
I used the following as a work-around for ICH6, but this should be
derived from fwh_idsel= parameter:
$ setpci -s 0:1f.0 0xd8.W=0xf0c0
After: (decode-enabled.txt)
0xfff80000/0xffb80000 FWH decode enabled
0xfff00000/0xffb00000 FWH decode enabled
0xffe80000/0xffa80000 FWH decode enabled
0xffe00000/0xffa00000 FWH decode enabled
Maximum FWH chip size: 0x200000 bytes
Regards,
Kyösti Mälkki
Allow easy disabling of all programmers except selected ones by setting
CONFIG_DEFAULT=no.
Setting CONFIG_DEFAULT=yes has no effect.
Usage example:
# make CONFIG_DUMMY=yes CONFIG_NIC3COM=yes CONFIG_DEFAULT=no
This will disable all programmers except dummy and nic3com.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Index: flashrom-Makefile_config_default/Makefile
===================================================================
--- flashrom-Makefile_config_default/Makefile (Revision 1623)
+++ flashrom-Makefile_config_default/Makefile (Arbeitskopie)
@@ -52,6 +52,10 @@
# Compilation will fail for unspecified values.
CONFIG_DEFAULT_PROGRAMMER ?= PROGRAMMER_INVALID
+# This parameter picks the default setting for CONFIG_ variables of programmers not specified explicitly
+# on the command line.
+CONFIG_DEFAULT ?= yes
+
# If your compiler spits out excessive warnings, run make WARNERROR=no
# You shouldn't have to change this flag.
WARNERROR ?= yes
@@ -322,65 +326,65 @@
SVNDEF := -D'FLASHROM_VERSION="$(VERSION)"'
# Always enable internal/onboard support for now.
-CONFIG_INTERNAL ?= yes
+CONFIG_INTERNAL ?= $(CONFIG_DEFAULT)
# Always enable serprog for now. Needs to be disabled on Windows.
-CONFIG_SERPROG ?= yes
+CONFIG_SERPROG ?= $(CONFIG_DEFAULT)
# RayeR SPIPGM hardware support
-CONFIG_RAYER_SPI ?= yes
+CONFIG_RAYER_SPI ?= $(CONFIG_DEFAULT)
# PonyProg2000 SPI hardware support
-CONFIG_PONY_SPI ?= yes
+CONFIG_PONY_SPI ?= $(CONFIG_DEFAULT)
# Always enable 3Com NICs for now.
-CONFIG_NIC3COM ?= yes
+CONFIG_NIC3COM ?= $(CONFIG_DEFAULT)
# Enable NVIDIA graphics cards. Note: write and erase do not work properly.
-CONFIG_GFXNVIDIA ?= yes
+CONFIG_GFXNVIDIA ?= $(CONFIG_DEFAULT)
# Always enable SiI SATA controllers for now.
-CONFIG_SATASII ?= yes
+CONFIG_SATASII ?= $(CONFIG_DEFAULT)
# Highpoint (HPT) ATA/RAID controller support.
# IMPORTANT: This code is not yet working!
CONFIG_ATAHPT ?= no
# Always enable FT2232 SPI dongles for now.
-CONFIG_FT2232_SPI ?= yes
+CONFIG_FT2232_SPI ?= $(CONFIG_DEFAULT)
# Always enable dummy tracing for now.
-CONFIG_DUMMY ?= yes
+CONFIG_DUMMY ?= $(CONFIG_DEFAULT)
# Always enable Dr. Kaiser for now.
-CONFIG_DRKAISER ?= yes
+CONFIG_DRKAISER ?= $(CONFIG_DEFAULT)
# Always enable Realtek NICs for now.
-CONFIG_NICREALTEK ?= yes
+CONFIG_NICREALTEK ?= $(CONFIG_DEFAULT)
# Disable National Semiconductor NICs until support is complete and tested.
CONFIG_NICNATSEMI ?= no
# Always enable Intel NICs for now.
-CONFIG_NICINTEL ?= yes
+CONFIG_NICINTEL ?= $(CONFIG_DEFAULT)
# Always enable SPI on Intel NICs for now.
-CONFIG_NICINTEL_SPI ?= yes
+CONFIG_NICINTEL_SPI ?= $(CONFIG_DEFAULT)
# Always enable SPI on OGP cards for now.
-CONFIG_OGP_SPI ?= yes
+CONFIG_OGP_SPI ?= $(CONFIG_DEFAULT)
# Always enable Bus Pirate SPI for now.
-CONFIG_BUSPIRATE_SPI ?= yes
+CONFIG_BUSPIRATE_SPI ?= $(CONFIG_DEFAULT)
# Disable Dediprog SF100 until support is complete and tested.
CONFIG_DEDIPROG ?= no
# Always enable Marvell SATA controllers for now.
-CONFIG_SATAMV ?= yes
+CONFIG_SATAMV ?= $(CONFIG_DEFAULT)
# Enable Linux spidev interface by default. We disable it on non-Linux targets.
-CONFIG_LINUX_SPI ?= yes
+CONFIG_LINUX_SPI ?= $(CONFIG_DEFAULT)
# Disable wiki printing by default. It is only useful if you have wiki access.
CONFIG_PRINT_WIKI ?= no
--
http://www.hailfinger.org/
