Am 25.07.2011 01:34 schrieb Stefan Tauner:
> On Fri, 22 Jul 2011 21:00:48 +0200
> Carl-Daniel Hailfinger <[email protected]> wrote:
>
>
>> ===================================================================
>> --- flashrom-ich_fwh_idsel_programmer_param_docfix/flashrom.8
>> (Revision 1380)
>> +++ flashrom-ich_fwh_idsel_programmer_param_docfix/flashrom.8
>> (Arbeitskopie)
>>
> i did not check the facts, but it looks ok. maybe a concrete example
> would be nice.
>
Done.
>> Using flashrom on laptops is dangerous and may easily make your hardware
>> unusable (see also the
>> .B BUGS
>> Index: flashrom-ich_fwh_idsel_programmer_param_docfix/chipset_enable.c
>> ===================================================================
>> --- flashrom-ich_fwh_idsel_programmer_param_docfix/chipset_enable.c
>> (Revision 1380)
>> +++ flashrom-ich_fwh_idsel_programmer_param_docfix/chipset_enable.c
>> (Arbeitskopie)
>> @@ -301,7 +301,7 @@
>>
>> static int enable_flash_ich_dc(struct pci_dev *dev, const char *name)
>> {
>> - uint32_t fwh_conf;
>> + uint64_t fwh_conf;
>>
> fwh_idsel?
> would increase the patch size though
>
I created a separate fwh_idsel variable with narrower scope.
>> int i;
>> char *idsel = NULL;
>> int tmp;
>> @@ -311,15 +311,18 @@
>>
>> idsel = extract_programmer_param("fwh_idsel");
>> if (idsel && strlen(idsel)) {
>> - fwh_conf = (uint32_t)strtoul(idsel, NULL, 0);
>> -
>> - /* FIXME: Need to undo this on shutdown. */
>> - msg_pinfo("\nSetting IDSEL=0x%x for top 16 MB", fwh_conf);
>> - rpci_write_long(dev, 0xd0, fwh_conf);
>> - rpci_write_word(dev, 0xd4, fwh_conf);
>> + fwh_conf = pci_read_long(dev, 0xd0);
>> + fwh_conf <<= 16;
>> + fwh_conf |= pci_read_word(dev, 0xd4);
>> + msg_pinfo("\nSetting IDSEL from 0x%012llx ", fwh_conf);
>> + /* Base 16, nothing else makes sense. */
>> + fwh_conf = (uint64_t)strtoull(idsel, NULL, 16);
>> + msg_pinfo("to 0x%012llx for top 16 MB", fwh_conf);
>>
> the "from to" message is a good idea imo, i am not sure if the verbosity
> level is the right choice. most of the time parameters are just applied
> without a message (but when there is an error). maybe msg_pdbg would be
> better? OTOH if i specify something on the cli i would like to know if
> it has changed anything or not (even in the default verbosity).
> so i personally would leave it as it is, but i am not so sure what
> carldani the output slayer would recommend ;)
>
Downgraded to dbg, thanks for noticing this.
> apart from that, this does not compile on my machine:
> error: format ‘%012llx’ expects type ‘long long unsigned int’, but argument 3
> has type ‘uint64_t’
>
Changed format specifier to PRIx64.
> also the strtoul result is not checked for errors.
> note from the man page:
>
> Since strtoul() can legitimately return 0 or LONG_MAX (LLONG_MAX for
> strtoull()) on both success and failure, the calling program should set
> errno to 0 before the call, and then determine if an error occurred
> by checking whether errno has a nonzero value after the call.
>
Done.
> you have also mentioned an idea to check for values >= (1<<48) this
> could be combined to one check/message or be done in two (one
> warning/info, one error).
>
Both are errors now.
Fix ICH FWH IDSEL setting with the fwh_idsel= internal programmer parameter.
The code took 32 bits of input and wrote them to an 48 bit register,
duplicating some values.
Document the fwh_idsel= parameter in the man page.
Signed-off-by: Carl-Daniel Hailfinger <[email protected]>
Index: flashrom-ich_fwh_idsel_programmer_param_docfix/flashrom.8
===================================================================
--- flashrom-ich_fwh_idsel_programmer_param_docfix/flashrom.8 (Revision 1380)
+++ flashrom-ich_fwh_idsel_programmer_param_docfix/flashrom.8 (Arbeitskopie)
@@ -298,6 +298,23 @@
.B it87spi
programmer.
.sp
+If you have an Intel chipset with an ICH6 or later southbridge and if you want
+to set specific IDSEL values for a non-default flash chip or an embedded
+controller (EC), you can use the
+.sp
+.B " flashrom \-p internal:fwh_idsel=value"
+.sp
+syntax where value is the 48-bit hexadecimal raw value to be written in the
+IDSEL registers of the Intel southbridge. The upper 32 bits use one hex digit
+each per 512 kB range between 0xffc00000 and 0xffffffff, and the lower 16 bits
+use one hex digit each per 1024 kB range between 0xff400000 and 0xff7fffff.
+The rightmost hex digit corresponds with the lowest address range. All address
+ranges have a corresponding sister range 4 MB below with identical IDSEL
+settings. The default value for ICH7 is given in the example below.
+.sp
+Example:
+.B "flashrom \-p internal:fwh_idsel=0x001122334567"
+.sp
Using flashrom on laptops is dangerous and may easily make your hardware
unusable (see also the
.B BUGS
Index: flashrom-ich_fwh_idsel_programmer_param_docfix/chipset_enable.c
===================================================================
--- flashrom-ich_fwh_idsel_programmer_param_docfix/chipset_enable.c
(Revision 1380)
+++ flashrom-ich_fwh_idsel_programmer_param_docfix/chipset_enable.c
(Arbeitskopie)
@@ -30,6 +30,8 @@
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
+#include <inttypes.h>
+#include <errno.h>
#include "flash.h"
#include "programmer.h"
@@ -311,15 +313,31 @@
idsel = extract_programmer_param("fwh_idsel");
if (idsel && strlen(idsel)) {
- fwh_conf = (uint32_t)strtoul(idsel, NULL, 0);
-
- /* FIXME: Need to undo this on shutdown. */
- msg_pinfo("\nSetting IDSEL=0x%x for top 16 MB", fwh_conf);
- rpci_write_long(dev, 0xd0, fwh_conf);
- rpci_write_word(dev, 0xd4, fwh_conf);
+ uint64_t fwh_idsel;
+ fwh_idsel = pci_read_long(dev, 0xd0);
+ fwh_idsel <<= 16;
+ fwh_idsel |= pci_read_word(dev, 0xd4);
+ msg_pdbg("\nSetting IDSEL from 0x%012" PRIx64 " ", fwh_idsel);
+ errno = 0;
+ /* Base 16, nothing else makes sense. */
+ fwh_idsel = (uint64_t)strtoull(idsel, NULL, 16);
+ if (errno) {
+ msg_perr("Error: fwh_idsel= specified, but value could "
+ "not be converted.\n");
+ goto idsel_garbage_out;
+ }
+ if (fwh_idsel & 0xffff000000000000ULL) {
+ msg_perr("Error: fwh_idsel= specified, but value had "
+ "unusued bits set.\n");
+ goto idsel_garbage_out;
+ }
+ msg_pdbg("to 0x%012llx for top 16 MB", fwh_idsel);
+ rpci_write_long(dev, 0xd0, (fwh_idsel >> 16) & 0xffffffff);
+ rpci_write_word(dev, 0xd4, fwh_idsel & 0xffff);
/* FIXME: Decode settings are not changed. */
} else if (idsel) {
- msg_perr("Error: idsel= specified, but no number given.\n");
+ msg_perr("Error: fwh_idsel= specified, but no value given.\n");
+idsel_garbage_out:
free(idsel);
/* FIXME: Return failure here once internal_init() starts
* to care about the return value of the chipset enable.
--
http://www.hailfinger.org/
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
