Am 01.07.2011 05:38 schrieb Stefan Tauner:
> - add ich_fill_data to fill the chipset registers from an array
> - add ich_read_data to copy the data from the chipset register into an array
> - replace the existing code with calls to those functions
>
> NB: this changes the semantic of the *run_opcode functions a bit:
> previously they could trash the chipset registers following the data registers because there
> was no (explicit) length check (it was and is checked by the dispatching run_opcode method).
>
> Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
>
I know you're just moving code, but this is a chance to clean up the
code and improve readability.
> diff --git a/ichspi.c b/ichspi.c
> index 19e52d2..905ed70 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -592,6 +592,57 @@ static void ich_set_bbar(uint32_t min_addr)
> msg_perr("Setting BBAR failed!\n");
> }
>
> +/* Reads up to len byte from the fdata/spid register into the data array.
> + * The amount actually read is limited by the maximum read size of the
> + * chipset. */
>
Can you put the terminating */ on its own line like the multiline
comments in other files?
> + static void ich_read_data(uint8_t *data, int len, int reg0_off)
> + {
> + int a;
>
int i would be a nicer name... although you just moved that code, so
it's not your fault.
> + uint32_t temp32 = 0;
> +
> + if (len > spi_programmer->max_data_read)
> + len = spi_programmer->max_data_read;
>
Can this really happen? If yes, does it depend on bugs elsewhere in the
code? This needs at least a msg_perr("bug in blabla, please report to
flashrom@"), we might even want to have the function return int to
handle errors.
> +
> + for (a = 0; a < len; a++) {
> + if ((a % 4) == 0)
> + temp32 = REGREAD32(reg0_off + (a));
>
Are the extra parentheses around a really needed?
> +
> + data[a] = (temp32 & (((uint32_t) 0xff) << ((a % 4) * 8)))
> + >> ((a % 4) * 8);
>
I think David had a suggestion on how to simplify this formula a bit.
> + }
> +}
> +
> +/* Fills up to len bytes from the data array into the fdata/spid registers.
> + * The amount actually written is limited by the maximum write size of the
> + * chipset and is returned by the function. */
>
The result is discarded anyway, and we probably shouldn't call this
function with a too big len in the first place, so a return type of void
may be the best choice.
That said, if you decide to care about the result of this function,
please make sure that ich_fill_data and ich_read_data have identical
return types and check that the comments above the functions match the
prototypes.
> +static uint8_t ich_fill_data(const uint8_t *data, int len, int reg0_off)
> +{
> + uint32_t temp32 = 0;
> + int a;
>
See ich_read_data.
> +
> + if (len > spi_programmer->max_data_write)
> + len = spi_programmer->max_data_write;
>
See ich_read_data.
> +
> + if (len <= 0)
> + return 0;
>
Superfluous check? len<0 should not happen, and len==0 won't execute the
for loop below.
> +
> + for (a = 0; a < len; a++) {
> + if ((a % 4) == 0) {
> + temp32 = 0;
> + }
>
Please kill superfluous {}.
> +
> + temp32 |= ((uint32_t) data[a]) << ((a % 4) * 8);
> +
> + if ((a % 4) == 3) {
> + REGWRITE32(reg0_off + (a - (a % 4)), temp32);
> + }
>
Please kill superfluous {}.
> + }
> + if (((a - 1) % 4) != 3) {
>
Yeargh! AFAIK modulo for negative integers is implementation-defined. So
your len<=0 check a few lines above may have been a good idea. That
said, it is a good idea to add a comment so others won't fall into that
trap.
And all this (a-1) stuff in the line above and below would be a lot more
readable if you inserted a--; before the if.
> + REGWRITE32(reg0_off + ((a - 1) - ((a - 1) % 4)), temp32);
> + }
>
Please kill superfluous {}.
> + return len;
> +}
> +
> /* This function generates OPCODES from or programs OPCODES to ICH according to
> * the chipset's SPI configuration lock.
> *
> @@ -638,9 +689,8 @@ static int ich7_run_opcode(OPCODE op, uint32_t offset,
> {
> int write_cmd = 0;
> int timeout;
> - uint32_t temp32 = 0;
> + uint32_t temp32;
> uint16_t temp16;
> - uint32_t a;
> uint64_t opmenu;
> int opcode_index;
>
> @@ -664,26 +714,8 @@ static int ich7_run_opcode(OPCODE op, uint32_t offset,
> REGWRITE32(ICH7_REG_SPIA, (offset & 0x00FFFFFF) | temp32);
>
> /* Program data into SPID0 to N */
> - if (write_cmd && (datalength != 0)) {
> - temp32 = 0;
> - for (a = 0; a < datalength; a++) {
> - if ((a % 4) == 0) {
> - temp32 = 0;
> - }
> -
> - temp32 |= ((uint32_t) data[a]) << ((a % 4) * 8);
> -
> - if ((a % 4) == 3) {
> - REGWRITE32(ICH7_REG_SPID0 + (a - (a % 4)),
> - temp32);
> - }
> - }
> - if (((a - 1) % 4) != 3) {
> - REGWRITE32(ICH7_REG_SPID0 +
> - ((a - 1) - ((a - 1) % 4)), temp32);
> - }
> -
> - }
> + if (write_cmd && (datalength != 0))
> + ich_fill_data(data, datalength, ICH7_REG_SPID0);
>
Nice! Killed superfluous {}.
>
> /* Assemble SPIS */
> temp16 = REGREAD16(ICH7_REG_SPIS);
> @@ -765,15 +797,7 @@ static int ich7_run_opcode(OPCODE op, uint32_t offset,
> }
>
> if ((!write_cmd) && (datalength != 0)) {
> - for (a = 0; a < datalength; a++) {
> - if ((a % 4) == 0) {
> - temp32 = REGREAD32(ICH7_REG_SPID0 + (a));
> - }
> -
> - data[a] =
> - (temp32 & (((uint32_t) 0xff) << ((a % 4) * 8)))
> - >> ((a % 4) * 8);
> - }
> + ich_read_data(data, datalength, ICH7_REG_SPID0);
>
Can you kill {} here as well?
> }
>
> return 0;
> @@ -785,7 +809,6 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset,
> int write_cmd = 0;
> int timeout;
> uint32_t temp32;
> - uint32_t a;
> uint64_t opmenu;
> int opcode_index;
>
> @@ -810,25 +833,8 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset,
> REGWRITE32(ICH9_REG_FADDR, (offset & 0x00FFFFFF) | temp32);
>
> /* Program data into FDATA0 to N */
> - if (write_cmd && (datalength != 0)) {
> - temp32 = 0;
> - for (a = 0; a < datalength; a++) {
> - if ((a % 4) == 0) {
> - temp32 = 0;
> - }
> -
> - temp32 |= ((uint32_t) data[a]) << ((a % 4) * 8);
> -
> - if ((a % 4) == 3) {
> - REGWRITE32(ICH9_REG_FDATA0 + (a - (a % 4)),
> - temp32);
> - }
> - }
> - if (((a - 1) % 4) != 3) {
> - REGWRITE32(ICH9_REG_FDATA0 +
> - ((a - 1) - ((a - 1) % 4)), temp32);
> - }
> - }
> + if (write_cmd && (datalength != 0))
> + ich_fill_data(data, datalength, ICH9_REG_FDATA0);
>
> /* Assemble SSFS + SSFC */
> temp32 = REGREAD32(ICH9_REG_SSFS);
> @@ -917,15 +923,7 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset,
> }
>
> if ((!write_cmd) && (datalength != 0)) {
> - for (a = 0; a < datalength; a++) {
> - if ((a % 4) == 0) {
> - temp32 = REGREAD32(ICH9_REG_FDATA0 + (a));
> - }
> -
> - data[a] =
> - (temp32 & (((uint32_t) 0xff) << ((a % 4) * 8)))
> - >> ((a % 4) * 8);
> - }
> + ich_read_data(data, datalength, ICH9_REG_FDATA0);
>
Kill superfluous {}
> }
>
> return 0;
>
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
Dear coreboot folks,
the Desktop Summit 2011 [1] will take place in Berlin from August 6th–
12th. The first three days will consist of keynotes and talks [2].
There seem to be some open slots on Monday and maybe on the weekend
evening too. I am CCing Lennart Poettering as one of the organizers(?).
Knowing that some coreboot or flashrom developers live in Berlin it
would be awesome if one of them could present coreboot or flashrom. For
coreboot a short introduction and demonstration of for example the
ThinkPad X60/T60 and ASRock E350M1 would be a good idea.
I know this is on short notice and the Desktop Summit is normally
associated with GNOME and KDE and therefore coreboot seems out of place.
But in my opinion – if accepted – coreboot could reach *developers* who
might be interested in the possibilities of coreboot and do not know
about this.
flashrom could be interesting for the desktop developers to know that
there are tools to upgrade the BIOS from the desktop and maybe someone
is motivated to code up a GUI (graphical user interface) for flashrom.
That is just a thought and I do not know how easy that would be and if
flashrom provides these kinds of means to be hooked up to a GUI.
I would do it, but I am only able to be there on Saturday and not being
that knowledgeable with the internals I would rather not do it.
Thanks,
Paul
[1] https://desktopsummit.org/
[2] https://desktopsummit.org/program
Author: stefanct
Date: Wed Jul 13 22:48:54 2011
New Revision: 1372
URL: http://flashrom.org/trac/flashrom/changeset/1372
Log:
enable writing on mcp6x_7x
this was deliberately disabled until now, but seems to work well enough.
Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Modified:
trunk/chipset_enable.c
Modified: trunk/chipset_enable.c
==============================================================================
--- trunk/chipset_enable.c Wed Jul 13 13:22:03 2011 (r1371)
+++ trunk/chipset_enable.c Wed Jul 13 22:48:54 2011 (r1372)
@@ -912,8 +912,9 @@
*/
buses_supported = CHIP_BUSTYPE_NONE;
msg_pdbg("Flash bus type is SPI\n");
- msg_perr("SPI on this chipset is WIP. Write is unsupported!\n");
- programmer_may_write = 0;
+ msg_pinfo("SPI on this chipset is WIP. Please report any "
+ "success or failure by mailing us the verbose "
+ "output to flashrom(a)flashrom.org, thanks!\n");
break;
default:
/* Should not happen. */
On Fri, Jul 01, 2011 at 06:05:07AM +0200, Stefan Tauner wrote:
> compile tested only.
> btw... why dont we wrap malloc to automatically include these checks?
> there is probably a patch somewhere that adds totally awesome shutdown code in OOM cases,
> but afaics we just print a warning and exit right now... it is just stupid to do the checks in the real
> code then.
>
> Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
It's definately better than what we have now (no error checking at all), so:
Acked-by: Uwe Hermann <uwe(a)hermann-uwe.de>
A few notes for later though:
- We should call any shutdown function which may need to be called in
the respective places (and any free()s which may be needed).
- Later we should not exit(1) upon the error, but rather return an
error code instead (e.g. FL_ERR_MALLOC or similar), at least in all
public "API" functions which are meant to be in the upcoming
libflashrom.
- A small xmalloc() wrapper (or use another name) would indeed be nice, too,
but once we really return error codes etc. it's not really useful
anymore I think.
Uwe.
--
http://hermann-uwe.de | http://sigrok.orghttp://randomprojects.org | http://unmaintained-free-software.org
Hello,
I successfully flash this motherboard with a "Winbond W39V040A" (512 kB,
LPC) chip which is currently tagged as UNTESTED for ERASE and WRITE.
Flash from BIOS version 1.0 to 1.A (the last one).
http://eu.msi.com/product/mb/K8MM-V.html
Here are the logs.
Benjamin
Dear Alexabder,
Am Montag, den 11.07.2011, 20:46 +0300 schrieb Senior College Krikoon Hilton Enterprise:
> I Alexabder
> I very hope and trust you
> How many time you need do this?
I am sorry I do not understand your question. What do we need to do?
> [root@localhost flashrom]# flashrom -V
> flashrom v0.9.3-r1299 on Linux 2.6.38.8-32.fc15.i686 (i686), built with
> libpci 3.1.7, GCC 4.6.0 20110428 (Red Hat 4.6.0-6), little endian
> flashrom is free software, get the source code at http://www.flashrom.org
[…]
If you want to upgrade you BIOS you should try a newer version [1][2]
and make sure you can recover from a failed flashing operation if your
motherboard is not marked as supported.
Thanks,
Paul
[1] http://flashrom.org/Flashrom
[2] http://flashrom.org/Downloads