Re: [flashrom] [PATCH] Fix ICH FWH IDSEL setting

[Stefan Tauner]

On Mon, 25 Jul 2011 08:48:37 +0200
Carl-Daniel Hailfinger  wrote:
> Signed-off-by: Carl-Daniel Hailfinger
> 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

having the example map something from the datasheet to the parameter is
excellent, thanks.
> 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);

should use PRIx64 here too.
> 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.

the output in the case of an error is now:
Setting IDSEL from 0x000000004567 Error: fwh_idsel= specified, but value could not be converted.

using fwh_idsel_new, fwh_idsel_old and deferred output would fix
that.