The patch is mostly ack-able, save for a few nits.
On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner <
stefan.tauner(a)student.tuwien.ac.at> wrote:
> My implementation does not defer the processing until doit(), but after the
> argument parsing loop only (doit() should not contain argument checks).
>
Just FYI -- The reason we deferred processing until doit() was because our
usage case calls for reading the ROM content to find pre-existing mappings
(e.g. fmap) which exist in the ROM image. It might be useful to eventually
do the same thing for cbfs and Intel's flash descriptor.
On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner <
stefan.tauner(a)student.tuwien.ac.at> wrote:
> This allows to specify -i and -l parameters in any order.
>
> I don't like the output in error cases much, any ideas?
> example of a good run:
> flashrom -p dummy:emulate=SST25VF032B -i normal -w
> ../testimages/4096kB.rand.img -l test.layout -i gfxrom -i fallback
> flashrom v0.9.4-r1396 on Linux 2.6.35-30-generic (x86_64), built with
> libpci 3.1.7, GCC 4.4.5, little endian
> flashrom is free software, get the source code at http://www.flashrom.org
>
> Using region(s): "normal", "gfxrom", "fallback".
> Calibrating delay loop... OK.
> Found SST flash chip "SST25VF032B" (4096 kB, SPI) on dummy.
> Reading old flash chip contents... done.
> Erasing and writing flash chip... Erase/write done.
> Verifying flash... VERIFIED.
>
Looks okay to me. And IIRC -V will list all ROM layout entries found even if
they are not included, which is nice for debugging.
On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner <
stefan.tauner(a)student.tuwien.ac.at> wrote:
> example of a semi-good run:
> flashrom -p dummy:emulate=SST25VF032B -i normal -w
> ../testimages/4096kB.rand.img -l test.layout -i gfxrom -i fallback
> flashrom v0.9.4-r1396 on Linux 2.6.35-30-generic (x86_64), built with
> libpci 3.1.7, GCC 4.4.5, little endian
> flashrom is free software, get the source code at http://www.flashrom.org
>
> Maximum number of ROM images (4) in layout file reached before end of
> layout file.
> Ignoring the rest of the layout file.
>
I think that if the user accidentally specifies an invalid layout or number
of included regions, then Flashrom should quit. Otherwise Flashrom might not
flash everything the user intends, leaving the content in an inconsistent
(probably bricked) state. This has become problematic due to complicated ROM
layouts in systems where multiple masters use a shared SPI flash (e.g. an EC
or Intel's ME insanity).
We can apply this fix in a follow-up patch if you'd prefer:
Index: layout.c
===================================================================
--- layout.c (revision 1402)
+++ layout.c (working copy)
@@ -159,7 +159,7 @@
"file reached before end of layout
file.\n",
MAX_ROMLAYOUT);
msg_gerr("Ignoring the rest of the layout file.\n");
- break;
+ return 1;
}
if (2 != fscanf(romlayout, "%s %s\n", tempstr,
rom_entries[romimages].name))
continue;
On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner <
stefan.tauner(a)student.tuwien.ac.at> wrote:
> example of a faulty run:
> flashrom -p dummy:emulate=SST25VF032B -i normal -w
> ../testimages/4096kB.rand.img -l test.layout -i gfxrom -i fallback -i bios
> flashrom v0.9.4-r1396 on Linux 2.6.35-30-generic (x86_64), built with
> libpci 3.1.7, GCC 4.4.5, little endian
> flashrom is free software, get the source code at http://www.flashrom.org
>
> Maximum number of ROM images (4) in layout file reached before end of
> layout file.
> Ignoring the rest of the layout file.
> Using region(s): "normal", "gfxrom", "fallback"Invalid region specified:
> "bios"
>
If you split that last line so that the "Invalid region" part shows up on a
newline, it looks okay.
Please run "flashrom --help" for usage info.
> Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
> ---
> cli_classic.c | 11 ++++-----
> flash.h | 2 +
> layout.c | 68
> ++++++++++++++++++++++++++++++++++++++++++++++++++------
> 3 files changed, 67 insertions(+), 14 deletions(-)
>
> diff --git a/cli_classic.c b/cli_classic.c
> index 36fe9ad..e44ea86 100644
> --- a/cli_classic.c
> +++ b/cli_classic.c
> @@ -233,14 +233,9 @@ int cli_classic(int argc, char *argv[])
> cli_classic_abort_usage();
> break;
> case 'i':
> - /* FIXME: -l has to be specified before -i. */
> tempstr = strdup(optarg);
> - if (find_romentry(tempstr) < 0) {
> - fprintf(stderr, "Error: image %s not found
> in "
> - "layout file or -i specified before
> "
> - "-l\n", tempstr);
> + if (register_include_arg(tempstr) < 0)
> cli_classic_abort_usage();
> - }
> break;
> case 'L':
> if (++operation_specified > 1) {
> @@ -325,6 +320,10 @@ int cli_classic(int argc, char *argv[])
> cli_classic_abort_usage();
> }
>
> + if (process_include_args() < 0) {
> + cli_classic_abort_usage();
> + }
> +
> /* FIXME: Print the actions flashrom will take. */
>
> if (list_supported) {
> diff --git a/flash.h b/flash.h
> index 5b49e9d..0848255 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -251,6 +251,8 @@ int print(int type, const char *fmt, ...)
> __attribute__((format(printf, 2, 3)));
> int cli_classic(int argc, char *argv[]);
>
> /* layout.c */
> +int register_include_arg(char *name);
> +int process_include_args(void);
> int read_romlayout(char *name);
> int find_romentry(char *name);
> int handle_romentries(struct flashchip *flash, uint8_t *oldcontents,
> uint8_t *newcontents);
> diff --git a/layout.c b/layout.c
> index d719a05..936e316 100644
> --- a/layout.c
> +++ b/layout.c
> @@ -41,6 +41,12 @@ typedef struct {
> char name[256];
> } romlayout_t;
>
> +/* include_args lists arguments specified at the command line with -i.
> They
> + * must be processed at some point so that desired regions are marked as
> + * "included" in the rom_entries list.
> + */
> +static char *include_args[MAX_ROMLAYOUT];
> +static int num_include_args = 0; /* the number of valid entries. */
> static romlayout_t rom_entries[MAX_ROMLAYOUT];
>
> #if CONFIG_INTERNAL == 1 /* FIXME: Move the whole block to cbtable.c? */
> @@ -194,6 +200,20 @@ int read_romlayout(char *name)
> }
> #endif
>
> +/* register an include argument (-i) for later processing */
> +int register_include_arg(char *name)
> +{
> + if (num_include_args >= MAX_ROMLAYOUT) {
> + msg_gerr("Too many regions included (%i).\n",
> num_include_args);
> + return -1;
> + }
> +
> + include_args[num_include_args] = name;
> + num_include_args++;
> + return num_include_args;
> +}
> +
> +/* returns the index of the entry (or a negative value if it is not found)
> */
> int find_romentry(char *name)
> {
> int i;
> @@ -201,20 +221,49 @@ int find_romentry(char *name)
> if (!romimages)
> return -1;
>
> - msg_ginfo("Looking for \"%s\"... ", name);
> -
> + msg_gspew("Looking for region \"%s\"... ", name);
> for (i = 0; i < romimages; i++) {
> if (!strcmp(rom_entries[i].name, name)) {
> rom_entries[i].included = 1;
> - msg_ginfo("found.\n");
> + msg_gspew("found.\n");
> return i;
> }
> }
> - msg_ginfo("not found.\n"); // Not found. Error.
> -
> + msg_gspew("not found.\n");
> return -1;
> }
>
> +/* process -i arguments
> + * returns 0 to indicate success, <0 to indicate failure
>
I think the usual Flashrom convention is to return 1 to indicate failure. I
used <0 out of habit. Let's go ahead and fix that coding convention for
upstream if desired, and update the call sites to do error checking
correctly.
> + */
> +int process_include_args(void)
> +{
> + int i;
> + unsigned int found = 0;
> + for (i = 0; i < num_include_args && include_args[i] != NULL; i++) {
> + /* User has specified an area, but no layout file is
> loaded. */
> + if (!romimages) {
> + msg_gerr("Region requested (with -i \"%s\"), "
> + "but no layout data is available.\n",
> + include_args[i]);
> + return -1;
>
return 1
+ }
> +
> + if (find_romentry(include_args[i]) < 0) {
> + msg_gerr("Invalid region specified: \"%s\"\n",
> + include_args[i]);
> + return -1;
>
return 1
> + }
> + if (found == 0)
> + msg_ginfo("Using region(s): \"%s\"",
> include_args[i]);
> + else
> + msg_ginfo(", \"%s\"", include_args[i]);
> + found++;
> + }
> + msg_ginfo(".\n");
> + return 0;
> +}
> +
> int find_next_included_romentry(unsigned int start)
> {
> int i;
> @@ -246,11 +295,12 @@ int handle_romentries(struct flashchip *flash,
> uint8_t *oldcontents, uint8_t *ne
> int entry;
> unsigned int size = flash->total_size * 1024;
>
> - /* If no layout file was specified or the layout file was empty,
> assume
> - * that the user wants to flash the complete new image.
> + /* If no regions were specified for inclusion, assume
> + * that the user wants to write the complete new image.
> */
> - if (!romimages)
> + if (num_include_args == 0)
> return 0;
> +
> /* Non-included romentries are ignored.
> * The union of all included romentries is used from the new image.
> */
> @@ -262,6 +312,8 @@ int handle_romentries(struct flashchip *flash, uint8_t
> *oldcontents, uint8_t *ne
> size - start);
> break;
> }
> +
> + /* For non-included region, copy from old content. */
> if (rom_entries[entry].start > start)
> memcpy(newcontents + start, oldcontents + start,
> rom_entries[entry].start - start);
> --
> 1.7.1
>
>
--
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.
Hi
First thanks for a great service.
I successfully tested a Intel NIC card Model Intel PRO/100 S desktop adapter
Chipset Intel 82550EY
Atmel flash chip "AT49BV512"
There is also an onboard Atmel 93C46 that could not be read with –p nicintel_spi?
Here is the edited results
pupa@pupa-IMBA-9454ISA:~$ sudo flashrom -R test.bin -p nicintel
Mapping Intel NIC control/status reg at 0xff8ff000, unaligned size 0x10.
Found Atmel flash chip "AT49BV512" (64 kB, Parallel) on nicintel.
Reading old flash chip contents... done.
Verifying flash... VERIFIED.
pupa@pupa-IMBA-9454ISA:~$ sudo flashrom -E -p nicintel
Calibrating delay loop... OK.
Found "Intel 82557/8/9/0/1 Ethernet Pro 100" (8086:1229, BDF 03:0d.0).
Mapping Intel NIC control/status reg at 0xff8ff000, unaligned size 0x10.
Found Atmel flash chip "AT49BV512" (64 kB, Parallel) on nicintel.
Erasing and writing flash chip... Erase/write done.
pupa@pupa-IMBA-9454ISA:~$ sudo flashrom -v test.bin -p nicintel
Calibrating delay loop... OK.
Mapping Intel NIC control/status reg at 0xff8ff000, unaligned size 0x10.
Found Atmel flash chip "AT49BV512" (64 kB, Parallel) on nicintel.
Reading old flash chip contents... done.
Verifying flash... VERIFY FAILED at 0x00000000! Expected=0x55, Read=0xff, failed byte count
from 0x00000000-0x0000ffff: 0xd0cf
pupa@pupa-IMBA-9454ISA:~$ sudo flashrom -w test.bin -p nicintel
Calibrating delay loop... OK.
Mapping Intel NIC control/status reg at 0xff8ff000, unaligned size 0x10.
Found Atmel flash chip "AT49BV512" (64 kB, Parallel) on nicintel.
Reading old flash chip contents... done.
Erasing and writing flash chip... Erase/write done.
Verifying flash... VERIFIED.
pupa@pupa-IMBA-9454ISA:~$ sudo flashrom -v test.bin -p nicintel
Calibrating delay loop... OK.
Found "Intel 82557/8/9/0/1 Ethernet Pro 100" (8086:1229, BDF 03:0d.0).
Mapping Intel NIC control/status reg at 0xff8ff000, unaligned size 0x10.
Found Atmel flash chip "AT49BV512" (64 kB, Parallel) on nicintel.
Reading old flash chip contents... done.
Verifying flash... VERIFIED.
Hi,
I was looking for some way to decrease the flashing time with Flashrom
for a 2MB SST SST25VF016B chip with a Dediprog SF100 programmer. After
taking a look at the datasheet, stefanct suggested to try to change the
write function for SST25VF016B to spi_aai_write, instead of
spi_chip_write_1.
(I had tried changing it to spi_chip_write_256 some time ago.
Unsurprisingly, the results were not so good)
I did some benchmark by writing random data on a chip already filled
with random data. I attached the results. I've run the benchmark two
times and got similar results on the two runs.
In short:
- with spi_aai_write, writing takes 56 minutes and seems to work reliably
- with spi_chip_write_1, writing takes 143 minutes
I also ran the test with -VV but the log is so big I can't possibly
send it to the mailing list. Here it is:
http://neetwork.net/up/misc/bench_vv
I also attached the patch I applied to Flashrom's trunk.
--
Noé Rubinstein
Proformatique (groupe Avencall) - XiVO IPBX OpenHardware
10 bis, rue Lucien VOILIN - 92800 Puteaux
hello frederic,
thanks for your patches.
i have not looked at it in detail yet (i'll leave that to those who are
more familiar with the IMC problem), but i have spotted quite some
coding style issues. it would be great if you could fix those.
i'll explain what i noticed inline.
On Mon, 1 Aug 2011 10:45:24 +0200
frederic.temporelli(a)bull.net wrote:
> diff -ur ../flashrom-0.9.4-r1394/sb600spi.c ./sb600spi.c
> --- ../flashrom-0.9.4-r1394/sb600spi.c 2011-05-11 13:07:07.000000000 -0400
> +++ ./sb600spi.c 2011-07-29 22:45:48.693159918 -0400
> @@ -5,6 +5,8 @@
> * Copyright (C) 2008 Joe Bao <Zheng.Bao(a)amd.com>
> * Copyright (C) 2008 Advanced Micro Devices, Inc.
> * Copyright (C) 2009, 2010 Carl-Daniel Hailfinger
> + * Copyright (C) 2011 Bull SAS
> + * (Written by Frederic Temporelli <frederic.temporelli(a)bull.net> for Bull SAS )
space before the closing parenthesis
> *
> * 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
> @@ -43,6 +45,63 @@
>
> static uint8_t *sb600_spibar = NULL;
>
> +/* reference to LPC/SPI PCI device,
> + * requested for requesting/releasing
> + * device access, shared with IMC
> + */
> +struct pci_dev *lpc_isa_bridge = NULL;
> +
> +
> +/*
> + * avoid interaction from IMC while we are working with LPC/SPI
> + * this is done by requesting HostOwnLPC register (LPC_ISA_BRIDGE, write 1 in register 0x98)
line too long, please split (and some punctuation would increase readability then)
> + * then we have to read this register.
> + * Loop until we have the ownership
> + * see doc AMD 44413 - AMD SP5100 Register Reference Guide
> + */
> +static int resquest_lpc_ownership (void)
> +{
> + uint8_t tmp;
> + int i = 0;
> +
> + if (lpc_isa_bridge == NULL)
> + return 1;
> +
> + pci_write_byte(lpc_isa_bridge, 0x98, 0x01);
> +
> + while ( (tmp = pci_read_byte (lpc_isa_bridge, 0x98)) == 0 ){
we don't use inner space around clauses like here '( (' and '0 )' usually.
also there is a space missing between ')' and '{'
> + pci_write_byte(lpc_isa_bridge, 0x98, 0x01);
> + if (++i > 1024) {
> + msg_perr("IMC did not release flash.\n");
> + return 1;
> + }
> +
> + }
> + msg_pspew("flash ownership after %i cycles.\n", i);
> + i = 0;
please drop the line above completely
> + return 0;
> +}
> +
> +
one line break between functions please
> +/*
> + * release LPC/SPI ownership (IMC can access to these resources)
> + * this is done by releasing HostOwnLPC register
> + * (write 0 in register 0x98)
> + */
> +static int release_lpc_ownership (void)
> +{
> +
> + if (lpc_isa_bridge == NULL)
> + return 1;
> +
spaces instead of tabs in the following lines:
> + pci_write_byte(lpc_isa_bridge, 0x98, 0x00);
> +
> + msg_pspew("flash ownership is released.\n");
> + return 0;
> +}
> +
> +
> +
one line break between functions please
> static void reset_internal_fifo_pointer(void)
> {
> mmio_writeb(mmio_readb(sb600_spibar + 2) | 0x10, sb600_spibar + 2);
> @@ -93,6 +152,8 @@
> const unsigned char *writearr, unsigned char *readarr)
> {
> int count;
spaces instead of tabs
> + int result = 0;
> +
> /* First byte is cmd which can not being sent through FIFO. */
> unsigned char cmd = *writearr++;
> unsigned int readoffby1;
> @@ -103,16 +164,26 @@
> msg_pspew("%s, cmd=%x, writecnt=%x, readcnt=%x\n",
> __func__, cmd, writecnt, readcnt);
>
> + if (resquest_lpc_ownership() != 0){
space missing between ')' and '{'
> + result = SPI_PROGRAMMER_ERROR;
> + msg_pinfo("can't take ownership of LPC/SPI");
> + goto return_status;
> + }
> +
> +
> if (readcnt > 8) {
> msg_pinfo("%s, SB600 SPI controller can not receive %d bytes, "
> "it is limited to 8 bytes\n", __func__, readcnt);
> - return SPI_INVALID_LENGTH;
> +
> + result = SPI_INVALID_LENGTH;
> + goto return_status;
> }
>
> if (writecnt > 8) {
> msg_pinfo("%s, SB600 SPI controller can not send %d bytes, "
> "it is limited to 8 bytes\n", __func__, writecnt);
> - return SPI_INVALID_LENGTH;
> + result = SPI_INVALID_LENGTH;
> + goto return_status;
> }
>
> /* This is a workaround for a bug in SB600 and SB700. If we only send
> @@ -142,7 +213,10 @@
> * the FIFO pointer to the first byte we want to send.
> */
> if (reset_compare_internal_fifo_pointer(writecnt))
> - return SPI_PROGRAMMER_ERROR;
> + {
the '{' should be in the same line as the 'if'
> + result = SPI_PROGRAMMER_ERROR;
> + goto return_status;
> + }
>
> msg_pspew("Executing: \n");
> execute_command();
> @@ -159,7 +233,10 @@
> * Usually, the chip will respond with 0x00 or 0xff.
> */
> if (reset_compare_internal_fifo_pointer(writecnt + readcnt))
> - return SPI_PROGRAMMER_ERROR;
> + {
here too
> + result = SPI_PROGRAMMER_ERROR;
> + goto return_status;
> + }
>
> /* Skip the bytes we sent. */
> msg_pspew("Skipping: ");
> @@ -168,8 +245,11 @@
> msg_pspew("[%02x]", cmd);
> }
> msg_pspew("\n");
> - if (compare_internal_fifo_pointer(writecnt))
> - return SPI_PROGRAMMER_ERROR;
> + if (compare_internal_fifo_pointer(writecnt)){
missing space
> +
> + result = SPI_PROGRAMMER_ERROR;
> + goto return_status;
> + }
>
> msg_pspew("Reading: ");
> for (count = 0; count < readcnt; count++, readarr++) {
> @@ -177,8 +257,11 @@
> msg_pspew("[%02x]", *readarr);
> }
> msg_pspew("\n");
> - if (reset_compare_internal_fifo_pointer(readcnt + writecnt))
> - return SPI_PROGRAMMER_ERROR;
> + if (reset_compare_internal_fifo_pointer(readcnt + writecnt)) {
> +
> + result = SPI_PROGRAMMER_ERROR;
> + goto return_status;
> + }
>
> if (mmio_readb(sb600_spibar + 1) != readwrite) {
> msg_perr("Unexpected change in SB600 read/write count!\n");
> @@ -186,10 +269,12 @@
> "causes random corruption.\nPlease stop all "
> "applications and drivers and IPMI which access the "
> "flash chip.\n");
> - return SPI_PROGRAMMER_ERROR;
> + result = SPI_PROGRAMMER_ERROR;
> }
>
> - return 0;
> +return_status:
> + release_lpc_ownership();
> + return result;
> }
>
> static const struct spi_programmer spi_programmer_sb600 = {
> @@ -211,6 +296,9 @@
> "Reserved", "33", "22", "16.5"
> };
>
> +
> + lpc_isa_bridge = dev;
> +
> /* Read SPI_BaseAddr */
> tmp = pci_read_long(dev, 0xa0);
> tmp &= 0xffffffe0; /* remove bits 4-0 (reserved) */
--
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
friend of mine allowed me to play with his dual fiber nic remotely.
it is an Intel PRO/1000 MF PCI-X card based on the 82545GM controller:
http://geizhals.at/119043http://download.intel.com/design/network/datashts/82545gm_ds.pdfhttp://download.intel.com/design/network/manuals/8254x_GBe_SDM.pdf
i was able to probe the attached chip (SST39VF020) after adding the PCI
ID, but reads returned all 0xFF. i need to verify that this is not
correct, but i am quite certain that there is a working bootrom in
there.
i have not looked too much at the code, commit log, ml archive or the
datasheet yet, maybe there is something obvious for you...?
why does flashrom detect the card twice although there is only one in
there?
should we add a maximum size field to the pcidev_status arrays that
hold pci programmers? i wanted to do something like that ever since i
discovered that the 3com nic have a 128 kB limit (after borrowing a
bunch of them to access a 256 kB chip...).
PS: i have also tried to add the other nic's PCI ID (82562EZ-based) to
no avail.
--
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
Hello,
Here's the second patch that will remove the write protection.
If this patch isn't applied, even if SPI owenrship is ok, flashrom won't be able to write BIOS
Best regards
--
Fred
Hello,
Here's a patch for using SPI from AMD SouthBridge (SB700, SP5100, ...) without having issue with Integrated MicroControler (IMC) .
This issue has been reported by Carl-Daniel Hailfinger in ChangeSet 1173
http://flashrom.org/trac/flashrom/changeset/1173
AMD is now providing details about SP5100 register in document 44413:
http://support.amd.com/us/Embedded_TechDocs/44413.pdf
In this document (rev 3.02), we can see that a register is in charge of managing access to LPC (p 271 and 283)
=> with this patch, we take LPC ownership before each set of commands to SPI.
Ownership is released when command is done.
I did a test by reading BIOS in flash, and this is working nicely (without this patch, I wasn't able to read the BIOS).
Now, I don't have components to restore the BIOS in case of failure, so write test hasn't been done (yet)...
Note: this patch doesn't remove the write protection,
I'll sent a second patch which will remove this protection
Best regards
--
Fred
