On Fri, 22 Jul 2011 21:00:48 +0200 Carl-Daniel Hailfinger <[email protected]> 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 <[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,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 _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
