[flashrom] [PATCH] Fix ICH FWH IDSEL setting

[Stefan Tauner]

On Fri, 22 Jul 2011 21:00:48 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> 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 <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,20 @@
>  .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.
> +.sp

i did not check the facts, but it looks ok. maybe a concrete example
would be nice.

>  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

>  	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 ;)

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’

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.

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).

> +		rpci_write_long(dev, 0xd0, (fwh_conf >> 16) & 0xffffffff);
> +		rpci_write_word(dev, 0xd4, fwh_conf & 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 number given.\n");
>  		free(idsel);
>  		/* FIXME: Return failure here once internal_init() starts
>  		 * to care about the return value of the chipset enable.
> 
> 


-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner