| Message ID | 1254737223-16129-22-git-send-email-yamahata@valinux.co.jp |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Oct 05, 2009 at 07:07:01PM +0900, Isaku Yamahata wrote: > qdevfy pci bridge, and initialize secondary bus and subordinate bus > in configuration space. Why do you initialize these? Should not guest do this? > And some clean up to use symbolic constant and to remove // comments. You also apparently did some unrelated changes. Pls document. > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > --- > hw/pci.c | 85 +++++++++++++++++++++++++++++++++++++++++++------------------- > hw/pci.h | 9 ++++++ > 2 files changed, 68 insertions(+), 26 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index af864c6..fb93b99 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -174,6 +174,7 @@ static PCIBus *pci_register_secondary_bus(PCIBus *parent, > bus->sub_bus = dev->config[PCI_SUBORDINATE_BUS]; > QLIST_INIT(&bus->child); > QLIST_INSERT_HEAD(&parent->child, bus, sibling); > + vmstate_register(-1, &vmstate_pcibus, bus); > return bus; > } > > @@ -1140,7 +1141,7 @@ typedef struct { > PCIBus *bus; > } PCIBridge; > > -static void pci_bridge_write_config(PCIDevice *d, > +void pci_bridge_write_config(PCIDevice *d, > uint32_t address, uint32_t val, int len) > { > PCIBridge *s = (PCIBridge *)d; > @@ -1193,35 +1194,67 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function) > return bus->devices[PCI_DEVFN(slot, function)]; > } > > -PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did, > - uint8_t sec_bus, uint8_t sub_bus, > - pci_map_irq_fn map_irq, const char *name) > +int pci_bridge_initfn(PCIDevice *pci_dev) > +{ > + uint8_t *pci_conf = pci_dev->config; We don't need this variable IMO. > + > + /* secondary bus, subordinate bus and vid/did will be set > + by pci_bridge_create_simple(), the caller of qdev_init() */ > + pci_conf[PCI_COMMAND] = PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER; > + pci_conf[PCI_STATUS] = PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK; command and status are words, so it is better to use pci_set_word here, even if specific values happen to be in the 1'st byte. > + pci_config_set_class(pci_conf, PCI_CLASS_BRIDGE_PCI); > + pci_conf[PCI_LATENCY_TIMER] = 0x10; Why 0x10? > + pci_conf[PCI_HEADER_TYPE] = > + PCI_HEADER_TYPE_MULTI_FUNCTION | PCI_HEADER_TYPE_BRIDGE; Any idea why it's multifunction? Many (most?) bridges have a single function ... > + pci_conf[PCI_PRIMARY_BUS] = pci_dev->bus->bus_num; I think this is 0 at reset, and assigned by BIOS. > + pci_conf[PCI_SEC_STATUS] = PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK; pci_set_word. > + > + return 0; > +} > + > +#define PCI_BRIDGE_DEFAULT "default PCI to PCI bridge" > +static PCIDeviceInfo pci_bridge_info_default = { > + .qdev.name = PCI_BRIDGE_DEFAULT, > + .qdev.size = sizeof(PCIBridge), > + .config_write = pci_bridge_write_config, > + .init = pci_bridge_initfn, > + .pcie = 0, > +}; > + > +static void pci_bridge_register_device(void) > { > - PCIBridge *s; > - s = (PCIBridge *)pci_register_device(bus, name, sizeof(PCIBridge), > - devfn, NULL, pci_bridge_write_config); > - > - pci_config_set_vendor_id(s->dev.config, vid); > - pci_config_set_device_id(s->dev.config, did); > - > - s->dev.config[0x04] = 0x06; // command = bus master, pci mem > - s->dev.config[0x05] = 0x00; > - s->dev.config[0x06] = 0xa0; // status = fast back-to-back, 66MHz, no error > - s->dev.config[0x07] = 0x00; // status = fast devsel > - s->dev.config[0x08] = 0x00; // revision > - s->dev.config[0x09] = 0x00; // programming i/f > - pci_config_set_class(s->dev.config, PCI_CLASS_BRIDGE_PCI); > - s->dev.config[0x0D] = 0x10; // latency_timer > - s->dev.config[PCI_HEADER_TYPE] = > - PCI_HEADER_TYPE_MULTI_FUNCTION | PCI_HEADER_TYPE_BRIDGE; // header_type > - s->dev.config[0x1E] = 0xa0; // secondary status > + pci_qdev_register(&pci_bridge_info_default); > +} > +device_init(pci_bridge_register_device); > + > +PCIBus *pci_bridge_create_simple(PCIBus *bus, int devfn, > + uint16_t vid, uint16_t did, > + uint8_t sec_bus, uint8_t sub_bus, > + pci_map_irq_fn map_irq, const char *bus_name, > + const char *name) > +{ > + PCIDevice *d; > + PCIBridge *br; > + > + d = pci_create_simple(bus, devfn, name); > > + pci_config_set_vendor_id(d->config, vid); > + pci_config_set_device_id(d->config, did); > assert(sec_bus <= sub_bus); > - s->dev.config[PCI_SECONDARY_BUS] = sec_bus; > - s->dev.config[PCI_SUBORDINATE_BUS] = sub_bus; > + d->config[PCI_SECONDARY_BUS] = sec_bus; > + d->config[PCI_SUBORDINATE_BUS] = sub_bus; > > - s->bus = pci_register_secondary_bus(bus, &s->dev, map_irq, name); > - return s->bus; > + br = DO_UPCAST(PCIBridge, dev, d); > + br->bus = pci_register_secondary_bus(bus, d, map_irq, name); > + return br->bus; > +} > + > +PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, > + uint16_t did, uint8_t sec_bus, uint8_t sub_bus, > + pci_map_irq_fn map_irq, const char *name) > +{ > + return pci_bridge_create_simple(bus, devfn, vid, did, sec_bus, sub_bus, > + map_irq, name, PCI_BRIDGE_DEFAULT); > } > > static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base) > diff --git a/hw/pci.h b/hw/pci.h > index 1d45437..5cf0b59 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -294,6 +294,15 @@ void pci_info(Monitor *mon); > PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did, > uint8_t sec_bus, uint8_t sub_bus, > pci_map_irq_fn map_irq, const char *name); > +PCIBus *pci_bridge_create_simple(PCIBus *bus, int devfn, > + uint16_t vid, uint16_t did, > + uint8_t sec_bus, uint8_t sub_bus, > + pci_map_irq_fn map_irq, const char *bus_name, > + const char *name); > +void pci_bridge_write_config(PCIDevice *d, > + uint32_t address, uint32_t val, int len); > +int pci_bridge_initfn(PCIDevice *pci_dev); > +void pci_bridge_set_map_irq(PCIBus *bus, pci_map_irq_fn map_irq); > > static inline void > pci_set_byte(uint8_t *config, uint8_t val) > -- > 1.6.0.2
On Mon, Oct 05, 2009 at 07:07:01PM +0900, Isaku Yamahata wrote:
> + br = DO_UPCAST(PCIBridge, dev, d);
By the way, why do we use DO_UPCAST thing,
which requires that field is at the start of parent
structure, instead of container_of which does not?
diff --git a/hw/pci.c b/hw/pci.c index af864c6..fb93b99 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -174,6 +174,7 @@ static PCIBus *pci_register_secondary_bus(PCIBus *parent, bus->sub_bus = dev->config[PCI_SUBORDINATE_BUS]; QLIST_INIT(&bus->child); QLIST_INSERT_HEAD(&parent->child, bus, sibling); + vmstate_register(-1, &vmstate_pcibus, bus); return bus; } @@ -1140,7 +1141,7 @@ typedef struct { PCIBus *bus; } PCIBridge; -static void pci_bridge_write_config(PCIDevice *d, +void pci_bridge_write_config(PCIDevice *d, uint32_t address, uint32_t val, int len) { PCIBridge *s = (PCIBridge *)d; @@ -1193,35 +1194,67 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function) return bus->devices[PCI_DEVFN(slot, function)]; } -PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did, - uint8_t sec_bus, uint8_t sub_bus, - pci_map_irq_fn map_irq, const char *name) +int pci_bridge_initfn(PCIDevice *pci_dev) +{ + uint8_t *pci_conf = pci_dev->config; + + /* secondary bus, subordinate bus and vid/did will be set + by pci_bridge_create_simple(), the caller of qdev_init() */ + pci_conf[PCI_COMMAND] = PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER; + pci_conf[PCI_STATUS] = PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK; + pci_config_set_class(pci_conf, PCI_CLASS_BRIDGE_PCI); + pci_conf[PCI_LATENCY_TIMER] = 0x10; + pci_conf[PCI_HEADER_TYPE] = + PCI_HEADER_TYPE_MULTI_FUNCTION | PCI_HEADER_TYPE_BRIDGE; + pci_conf[PCI_PRIMARY_BUS] = pci_dev->bus->bus_num; + pci_conf[PCI_SEC_STATUS] = PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK; + + return 0; +} + +#define PCI_BRIDGE_DEFAULT "default PCI to PCI bridge" +static PCIDeviceInfo pci_bridge_info_default = { + .qdev.name = PCI_BRIDGE_DEFAULT, + .qdev.size = sizeof(PCIBridge), + .config_write = pci_bridge_write_config, + .init = pci_bridge_initfn, + .pcie = 0, +}; + +static void pci_bridge_register_device(void) { - PCIBridge *s; - s = (PCIBridge *)pci_register_device(bus, name, sizeof(PCIBridge), - devfn, NULL, pci_bridge_write_config); - - pci_config_set_vendor_id(s->dev.config, vid); - pci_config_set_device_id(s->dev.config, did); - - s->dev.config[0x04] = 0x06; // command = bus master, pci mem - s->dev.config[0x05] = 0x00; - s->dev.config[0x06] = 0xa0; // status = fast back-to-back, 66MHz, no error - s->dev.config[0x07] = 0x00; // status = fast devsel - s->dev.config[0x08] = 0x00; // revision - s->dev.config[0x09] = 0x00; // programming i/f - pci_config_set_class(s->dev.config, PCI_CLASS_BRIDGE_PCI); - s->dev.config[0x0D] = 0x10; // latency_timer - s->dev.config[PCI_HEADER_TYPE] = - PCI_HEADER_TYPE_MULTI_FUNCTION | PCI_HEADER_TYPE_BRIDGE; // header_type - s->dev.config[0x1E] = 0xa0; // secondary status + pci_qdev_register(&pci_bridge_info_default); +} +device_init(pci_bridge_register_device); + +PCIBus *pci_bridge_create_simple(PCIBus *bus, int devfn, + uint16_t vid, uint16_t did, + uint8_t sec_bus, uint8_t sub_bus, + pci_map_irq_fn map_irq, const char *bus_name, + const char *name) +{ + PCIDevice *d; + PCIBridge *br; + + d = pci_create_simple(bus, devfn, name); + pci_config_set_vendor_id(d->config, vid); + pci_config_set_device_id(d->config, did); assert(sec_bus <= sub_bus); - s->dev.config[PCI_SECONDARY_BUS] = sec_bus; - s->dev.config[PCI_SUBORDINATE_BUS] = sub_bus; + d->config[PCI_SECONDARY_BUS] = sec_bus; + d->config[PCI_SUBORDINATE_BUS] = sub_bus; - s->bus = pci_register_secondary_bus(bus, &s->dev, map_irq, name); - return s->bus; + br = DO_UPCAST(PCIBridge, dev, d); + br->bus = pci_register_secondary_bus(bus, d, map_irq, name); + return br->bus; +} + +PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, + uint16_t did, uint8_t sec_bus, uint8_t sub_bus, + pci_map_irq_fn map_irq, const char *name) +{ + return pci_bridge_create_simple(bus, devfn, vid, did, sec_bus, sub_bus, + map_irq, name, PCI_BRIDGE_DEFAULT); } static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base) diff --git a/hw/pci.h b/hw/pci.h index 1d45437..5cf0b59 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -294,6 +294,15 @@ void pci_info(Monitor *mon); PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did, uint8_t sec_bus, uint8_t sub_bus, pci_map_irq_fn map_irq, const char *name); +PCIBus *pci_bridge_create_simple(PCIBus *bus, int devfn, + uint16_t vid, uint16_t did, + uint8_t sec_bus, uint8_t sub_bus, + pci_map_irq_fn map_irq, const char *bus_name, + const char *name); +void pci_bridge_write_config(PCIDevice *d, + uint32_t address, uint32_t val, int len); +int pci_bridge_initfn(PCIDevice *pci_dev); +void pci_bridge_set_map_irq(PCIBus *bus, pci_map_irq_fn map_irq); static inline void pci_set_byte(uint8_t *config, uint8_t val)
qdevfy pci bridge, and initialize secondary bus and subordinate bus in configuration space. And some clean up to use symbolic constant and to remove // comments. Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/pci.c | 85 +++++++++++++++++++++++++++++++++++++++++++------------------- hw/pci.h | 9 ++++++ 2 files changed, 68 insertions(+), 26 deletions(-)