On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> 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 < c-d.hailfinger.devel.2006@gmx.net> 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 < c-d.hailfinger.devel.2006@gmx.net> 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 < c-d.hailfinger.devel.2006@gmx.net> 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 < c-d.hailfinger.devel.2006@gmx.net> 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 < c-d.hailfinger.devel.2006@gmx.net> 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.