On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger < [email protected]> wrote:
> Undo all PCI config space writes on shutdown. > This means all chipset enables etc. will be undone on shutdown. > Any writes which are one-shot should use the permanent ppci_write_* > variants. > Nack. I like the idea of the rpci_* functions, but I think rpci_* should be opt-in and pci_* functions should remain as they are. As Michael pointed out, I don't think it's good to redefine this behavior, and will probably be a source of errors in the future for those used to pci_* functions as defined in libpci. On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger < [email protected]> wrote: > Index: flashrom-pci_configspace_shutdown_restore/drkaiser.c > =================================================================== > --- flashrom-pci_configspace_shutdown_restore/drkaiser.c (Revision > 1225) > +++ flashrom-pci_configspace_shutdown_restore/drkaiser.c > (Arbeitskopie) > @@ -61,8 +61,7 @@ > > int drkaiser_shutdown(void) > { > - /* Write protect the flash again. */ > - pci_write_word(pcidev_dev, PCI_MAGIC_DRKAISER_ADDR, 0); > + /* Flash write is disabled automatically by PCI restore. */ > It's fine to nuke this function. However, I'd rather see corresponding pci_write_word in drkaiser_init() changed to rpci_write_word(). On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger < [email protected]> wrote: > +#define NO_PCI_REDIRECT 1 > Should this be "NO_PCI_RESTORE"? Maybe I misunderstand the context of this particular change... On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger < [email protected]> wrote: > +#define register_undo_pci_write(a, b, c) \ > +{ \ > + struct undo_pci_write_data *undo_pci_write_data; \ > + undo_pci_write_data = malloc(sizeof(struct undo_pci_write_data)); \ > + undo_pci_write_data->dev = *a; \ > + undo_pci_write_data->pos = b; \ > + undo_pci_write_data->type = pci_write_type_##c; \ > + undo_pci_write_data->c##data = pci_read_##c(dev, pos); \ > + register_shutdown(undo_pci_write, undo_pci_write_data); \ > +} > + > +int rpci_write_byte(struct pci_dev *dev, int pos, uint8_t data) > +{ > + register_undo_pci_write(dev, pos, byte); > + return pci_write_byte(dev, pos, data); > +} > + > +int rpci_write_word(struct pci_dev *dev, int pos, uint16_t data) > +{ > + register_undo_pci_write(dev, pos, word); > + return pci_write_word(dev, pos, data); > +} > + > +int rpci_write_long(struct pci_dev *dev, int pos, uint32_t data) > +{ > + register_undo_pci_write(dev, pos, long); > + return pci_write_long(dev, pos, data); > +} > + > +int ppci_write_byte(struct pci_dev *dev, int pos, u8 data) > +{ > + return pci_write_byte(dev, pos, data); > +} > + > +int ppci_write_word(struct pci_dev *dev, int pos, u16 data) > +{ > + return pci_write_word(dev, pos, data); > +} > + > +int ppci_write_long(struct pci_dev *dev, int pos, u32 data) > +{ > + return pci_write_long(dev, pos, data); > +} > + > Dumb question -- What exactly is "pos" in this context? It looks like register offset, in which case "reg" or something would be a better name. On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger < [email protected]> wrote: > Index: flashrom-pci_configspace_shutdown_restore/gfxnvidia.c > =================================================================== > --- flashrom-pci_configspace_shutdown_restore/gfxnvidia.c (Revision > 1225) > +++ flashrom-pci_configspace_shutdown_restore/gfxnvidia.c > (Arbeitskopie) > @@ -89,13 +89,9 @@ > > int gfxnvidia_shutdown(void) > { > - uint32_t reg32; > - > - /* Disallow access to flash interface (and re-enable screen). */ > - reg32 = pci_read_long(pcidev_dev, 0x50); > - reg32 |= (1 << 0); > - pci_write_long(pcidev_dev, 0x50, reg32); > - > + /* Flash interface access is disabled (and screen enabled) > automatically > + * by PCI restore. > + */ > pci_cleanup(pacc); > release_io_perms(); > return 0; > Index: flashrom-pci_configspace_shutdown_restore/atahpt.c > =================================================================== > --- flashrom-pci_configspace_shutdown_restore/atahpt.c (Revision 1225) > +++ flashrom-pci_configspace_shutdown_restore/atahpt.c (Arbeitskopie) > @@ -59,13 +59,7 @@ > > int atahpt_shutdown(void) > { > - uint32_t reg32; > - > - /* Disable flash access again. */ > - reg32 = pci_read_long(pcidev_dev, REG_FLASH_ACCESS); > - reg32 &= ~(1 << 24); > - pci_write_long(pcidev_dev, REG_FLASH_ACCESS, reg32); > - > + /* Flash access is disabled automatically by PCI restore. */ > pci_cleanup(pacc); > release_io_perms(); > return 0; > Index: flashrom-pci_configspace_shutdown_restore/chipset_enable.c > =================================================================== > --- flashrom-pci_configspace_shutdown_restore/chipset_enable.c (Revision > 1225) > +++ flashrom-pci_configspace_shutdown_restore/chipset_enable.c > (Arbeitskopie) > @@ -498,17 +498,6 @@ > return enable_flash_ich_dc_spi(dev, name, 10); > } > > -static void via_do_byte_merge(void * arg) > -{ > - struct pci_dev * dev = arg; > - uint8_t val; > - > - msg_pdbg("Re-enabling byte merging\n"); > - val = pci_read_byte(dev, 0x71); > - val |= 0x40; > - pci_write_byte(dev, 0x71, val); > -} > - > static int via_no_byte_merge(struct pci_dev *dev, const char *name) > { > uint8_t val; > @@ -519,7 +508,6 @@ > msg_pdbg("Disabling byte merging\n"); > val &= ~0x40; > pci_write_byte(dev, 0x71, val); > - register_shutdown(via_do_byte_merge, dev); > } > return NOT_DONE_YET; /* need to find south bridge, too */ > } > Good stuff! On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger < [email protected]> wrote: > Index: flashrom-pci_configspace_shutdown_restore/flashrom.c > +#if NO_PCI_REDIRECT > +/* Don't touch pci_write_* definitions. */ > +#else > +#define pci_write_byte rpci_write_byte > +#define pci_write_word rpci_write_word > +#define pci_write_long rpci_write_long > #endif > +#endif This is really the only part of the patch that I dislike... I think the added work of changing a couple pci_write_* calls to rpci_* in the via, atahpt, drkaiser, and gfxnvidia functions is better than changing the behavior of pci_write_* all together. -- David Hendricks (dhendrix) Systems Software Engineer, Google Inc.
_______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
