[flashrom] [PATCH] Fix ICH FWH IDSEL setting

[Stefan Tauner]

On Mon, 25 Jul 2011 08:48:37 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
> 
> 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.
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner