On Mon, Feb 08, 2010 at 03:56:29PM -0600, Anthony Liguori wrote: > On 02/08/2010 03:01 PM, Michael S. Tsirkin wrote: >>> Sorry, but: >>> >>> versatile_pci.c: d->config[0x04] = 0x00; >>> versatile_pci.c: d->config[0x05] = 0x00; >>> versatile_pci.c: d->config[0x06] = 0x20; >>> versatile_pci.c: d->config[0x07] = 0x02; >>> >>> To: >>> >>> pci_config_set_command(d, 0); >>> pci_config_set_status(d, PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ); >>> >>> Is a huge improvement. >>> >> >> Yes but >> >> pci_set_word(d->config + PCI_COMMAND, 0); >> pci_set_word(d->config + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM | >> PCI_STATUS_66MHZ); >> >> is not much worse, and that API is already there. >> And advatage is it uses macros from linux which have >> higher chance to be correct than what we come up with. >> > > Oh, pci_set_word() is certainly an improvement. Personally, I prefer > passing the PCIDevice as a context and adding individual accessors but > anything is better than open coded config. > >>> I'm staring at a PCI config space diagram right >>> now and I'm *still* not even sure I'm interpreting the versatile_pci >>> code correctly :-) >>> >> I spent time cleaning up devices, just did not get to bridges. >> What I did is write patches and verify that compiled code >> did not change at all. This guarantees no breakage. >> Care to volunteer to complete that work? >> Separately people that are familiar with device can clean it up. >> > > It's on my radar
Just converted versatile_pci, with some nudging I might do others :) > but I've got another PCI series in flight. I've got a > branch pci-cleanup on staging that's a work in progress for adding a > proper region API along with PCI memory accessors. > > Once I find a little more time to finish converting VGA devices, I'll post. > > Regards, > > Anthony Liguori Great! That's required for proper spec compliance. VGA devices are definitely the main pain point here. -- MST
