<div class="gmail_quote">On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

Undo all PCI config space writes on shutdown.<br>
This means all chipset enables etc. will be undone on shutdown.<br>
Any writes which are one-shot should use the permanent ppci_write_*<br>
variants.<br></blockquote><div><br></div><div>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.</div>

<div><br></div><div>On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

Index: flashrom-pci_configspace_shutdown_restore/drkaiser.c<br>
===================================================================<br>
--- flashrom-pci_configspace_shutdown_restore/drkaiser.c        (Revision 1225)<br>
+++ flashrom-pci_configspace_shutdown_restore/drkaiser.c        (Arbeitskopie)<br>
@@ -61,8 +61,7 @@<br>
<br>
 int drkaiser_shutdown(void)<br>
 {<br>
-       /* Write protect the flash again. */<br>
-       pci_write_word(pcidev_dev, PCI_MAGIC_DRKAISER_ADDR, 0);<br>
+       /* Flash write is disabled automatically by PCI restore. */<br></blockquote><div><br></div><div>It's fine to nuke this function. However, I'd rather see corresponding pci_write_word in drkaiser_init() changed to rpci_write_word().</div>

<div><br></div><div>On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

+#define NO_PCI_REDIRECT 1<br></blockquote><div><br></div><div>Should this be "NO_PCI_RESTORE"? Maybe I misunderstand the context of this particular change...</div><div><br></div><div>On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote: </div>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">+#define register_undo_pci_write(a, b, c)                               \<br>
+{                                                                      \<br>
+       struct undo_pci_write_data *undo_pci_write_data;                \<br>
+       undo_pci_write_data = malloc(sizeof(struct undo_pci_write_data)); \<br>
+       undo_pci_write_data->dev = *a;                                  \<br>
+       undo_pci_write_data->pos = b;                                   \<br>
+       undo_pci_write_data->type = pci_write_type_##c;                 \<br>
+       undo_pci_write_data->c##data = pci_read_##c(dev, pos);          \<br>
+       register_shutdown(undo_pci_write, undo_pci_write_data);         \<br>
+}<br>
+<br>
+int rpci_write_byte(struct pci_dev *dev, int pos, uint8_t data)<br>
+{<br>
+       register_undo_pci_write(dev, pos, byte);<br>
+       return pci_write_byte(dev, pos, data);<br>
+}<br>
+<br>
+int rpci_write_word(struct pci_dev *dev, int pos, uint16_t data)<br>
+{<br>
+       register_undo_pci_write(dev, pos, word);<br>
+       return pci_write_word(dev, pos, data);<br>
+}<br>
+<br>
+int rpci_write_long(struct pci_dev *dev, int pos, uint32_t data)<br>
+{<br>
+       register_undo_pci_write(dev, pos, long);<br>
+       return pci_write_long(dev, pos, data);<br>
+}<br>
+<br>
+int ppci_write_byte(struct pci_dev *dev, int pos, u8 data)<br>
+{<br>
+       return pci_write_byte(dev, pos, data);<br>
+}<br>
+<br>
+int ppci_write_word(struct pci_dev *dev, int pos, u16 data)<br>
+{<br>
+       return pci_write_word(dev, pos, data);<br>
+}<br>
+<br>
+int ppci_write_long(struct pci_dev *dev, int pos, u32 data)<br>
+{<br>
+       return pci_write_long(dev, pos, data);<br>
+}<br>
+<br></blockquote><div><br></div><div>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.</div><div><br></div>

<div>On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">


Index: flashrom-pci_configspace_shutdown_restore/gfxnvidia.c<br>
===================================================================<br>
--- flashrom-pci_configspace_shutdown_restore/gfxnvidia.c       (Revision 1225)<br>
+++ flashrom-pci_configspace_shutdown_restore/gfxnvidia.c       (Arbeitskopie)<br>
@@ -89,13 +89,9 @@<br>
<br>
 int gfxnvidia_shutdown(void)<br>
 {<br>
-       uint32_t reg32;<br>
-<br>
-       /* Disallow access to flash interface (and re-enable screen). */<br>
-       reg32 = pci_read_long(pcidev_dev, 0x50);<br>
-       reg32 |= (1 << 0);<br>
-       pci_write_long(pcidev_dev, 0x50, reg32);<br>
-<br>
+       /* Flash interface access is disabled (and screen enabled) automatically<br>
+        * by PCI restore.<br>
+        */<br>
        pci_cleanup(pacc);<br>
        release_io_perms();<br>
        return 0;<br>
Index: flashrom-pci_configspace_shutdown_restore/atahpt.c<br>
===================================================================<br>
--- flashrom-pci_configspace_shutdown_restore/atahpt.c  (Revision 1225)<br>
+++ flashrom-pci_configspace_shutdown_restore/atahpt.c  (Arbeitskopie)<br>
@@ -59,13 +59,7 @@<br>
<br>
 int atahpt_shutdown(void)<br>
 {<br>
-       uint32_t reg32;<br>
-<br>
-       /* Disable flash access again. */<br>
-       reg32 = pci_read_long(pcidev_dev, REG_FLASH_ACCESS);<br>
-       reg32 &= ~(1 << 24);<br>
-       pci_write_long(pcidev_dev, REG_FLASH_ACCESS, reg32);<br>
-<br>
+       /* Flash access is disabled automatically by PCI restore. */<br>
        pci_cleanup(pacc);<br>
        release_io_perms();<br>
        return 0;<br>
Index: flashrom-pci_configspace_shutdown_restore/chipset_enable.c<br>
===================================================================<br>
--- flashrom-pci_configspace_shutdown_restore/chipset_enable.c  (Revision 1225)<br>
+++ flashrom-pci_configspace_shutdown_restore/chipset_enable.c  (Arbeitskopie)<br>
@@ -498,17 +498,6 @@<br>
        return enable_flash_ich_dc_spi(dev, name, 10);<br>
 }<br>
<br>
-static void via_do_byte_merge(void * arg)<br>
-{<br>
-       struct pci_dev * dev = arg;<br>
-       uint8_t val;<br>
-<br>
-       msg_pdbg("Re-enabling byte merging\n");<br>
-       val = pci_read_byte(dev, 0x71);<br>
-       val |= 0x40;<br>
-       pci_write_byte(dev, 0x71, val);<br>
-}<br>
-<br>
 static int via_no_byte_merge(struct pci_dev *dev, const char *name)<br>
 {<br>
        uint8_t val;<br>
@@ -519,7 +508,6 @@<br>
                msg_pdbg("Disabling byte merging\n");<br>
                val &= ~0x40;<br>
                pci_write_byte(dev, 0x71, val);<br>
-               register_shutdown(via_do_byte_merge, dev);<br>
        }<br>
        return NOT_DONE_YET;    /* need to find south bridge, too */<br>
 }<br></blockquote><div><br></div><div>Good stuff!</div><div><br></div><div>On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote: </div>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Index: flashrom-pci_configspace_shutdown_restore/flashrom.c<br>+#if NO_PCI_REDIRECT<br>
+/* Don't touch pci_write_* definitions. */<br>
+#else<br>
+#define pci_write_byte rpci_write_byte<br>
+#define pci_write_word rpci_write_word<br>
+#define pci_write_long rpci_write_long<br>
 #endif<br>
+#endif</blockquote><div><br></div><div>This is really the only part of the patch that I dislike...</div><div><br></div><div>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.</div>

<div><br></div></div>-- <br>David Hendricks (dhendrix)<br>Systems Software Engineer, Google Inc.<br>