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.
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().
+#define NO_PCI_REDIRECT 1
Should this be "NO_PCI_RESTORE"? Maybe I misunderstand the context of this particular change...
+#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.
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!
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.