| Message ID | ca619226f7c9d07f434d59df49b0a708d94e2071.1276573899.git.yamahata@valinux.co.jp |
|---|---|
| State | New |
| Headers | show |
On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote: > Don't overwrite pci header type. > Otherwise, multi function bit which pci_init_header_type() sets > appropriately is lost. > Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary to zero > which is already zero cleared. > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> ... > diff --git a/hw/apb_pci.c b/hw/apb_pci.c > index 31c8d70..cdf3bc2 100644 > --- a/hw/apb_pci.c > +++ b/hw/apb_pci.c > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d) > PCI_STATUS_DEVSEL_MEDIUM); > pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); > pci_set_byte(d->config + PCI_HEADER_TYPE, > - PCI_HEADER_TYPE_NORMAL); > + (pci_get_byte(d->config + PCI_HEADER_TYPE) & > + PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL); what is this doing?
On Tue, 15 Jun 2010, Isaku Yamahata wrote: > Don't overwrite pci header type. > Otherwise, multi function bit which pci_init_header_type() sets > appropriately is lost. > Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary to zero > which is already zero cleared. ac97 changes are fine with me [..snip..]
On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote: > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote: > > Don't overwrite pci header type. > > Otherwise, multi function bit which pci_init_header_type() sets > > appropriately is lost. > > Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary to zero > > which is already zero cleared. > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > ... > > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c > > index 31c8d70..cdf3bc2 100644 > > --- a/hw/apb_pci.c > > +++ b/hw/apb_pci.c > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d) > > PCI_STATUS_DEVSEL_MEDIUM); > > pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); > > pci_set_byte(d->config + PCI_HEADER_TYPE, > > - PCI_HEADER_TYPE_NORMAL); > > + (pci_get_byte(d->config + PCI_HEADER_TYPE) & > > + PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL); > > what is this doing? It changes the header type to normal device(bit 1-7) without overwriting multi function bit(bit 8). Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo, on the other hand pbc_pci_host_init() sets the register to PCI_HEADER_TYPE_NORMAL. To be honest I don't know why it does so, but that is what Blue wants. So I touch only multi function bit(bit 8) and leave other bit (bit 1-7) unchanged. If you don't like this hunk, I'll drop this hunk and leave it to Blue. What do you think? static PCIDeviceInfo pbm_pci_host_info = { .qdev.name = "pbm", .qdev.size = sizeof(PCIDevice), .init = pbm_pci_host_init, .header_type = PCI_HEADER_TYPE_BRIDGE, <<<<< Here };
On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote: > On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote: > > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote: > > > Don't overwrite pci header type. > > > Otherwise, multi function bit which pci_init_header_type() sets > > > appropriately is lost. > > > Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary to zero > > > which is already zero cleared. > > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > > > ... > > > > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c > > > index 31c8d70..cdf3bc2 100644 > > > --- a/hw/apb_pci.c > > > +++ b/hw/apb_pci.c > > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d) > > > PCI_STATUS_DEVSEL_MEDIUM); > > > pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); > > > pci_set_byte(d->config + PCI_HEADER_TYPE, > > > - PCI_HEADER_TYPE_NORMAL); > > > + (pci_get_byte(d->config + PCI_HEADER_TYPE) & > > > + PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL); > > > > what is this doing? > > It changes the header type to normal device(bit 1-7) without overwriting > multi function bit(bit 8). Don't we know what the multi function bit value is? > Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo, > on the other hand pbc_pci_host_init() sets the register > to PCI_HEADER_TYPE_NORMAL. > To be honest I don't know why it does so, but that is what Blue wants. BTW I think it would be prettier to have is_bridge instead of header_type as a qdev property. Agree? > So I touch only multi function bit(bit 8) and leave other bit (bit 1-7) > unchanged. > > If you don't like this hunk, I'll drop this hunk and leave it to Blue. > What do you think? Blue Swirl, could you comment on this please? > static PCIDeviceInfo pbm_pci_host_info = { > .qdev.name = "pbm", > .qdev.size = sizeof(PCIDevice), > .init = pbm_pci_host_init, > .header_type = PCI_HEADER_TYPE_BRIDGE, <<<<< Here > }; > > -- > yamahata
On Wed, Jun 16, 2010 at 11:54:25AM +0300, Michael S. Tsirkin wrote: > On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote: > > On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote: > > > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote: > > > > Don't overwrite pci header type. > > > > Otherwise, multi function bit which pci_init_header_type() sets > > > > appropriately is lost. > > > > Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary to zero > > > > which is already zero cleared. > > > > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > > > > > ... > > > > > > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c > > > > index 31c8d70..cdf3bc2 100644 > > > > --- a/hw/apb_pci.c > > > > +++ b/hw/apb_pci.c > > > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d) > > > > PCI_STATUS_DEVSEL_MEDIUM); > > > > pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); > > > > pci_set_byte(d->config + PCI_HEADER_TYPE, > > > > - PCI_HEADER_TYPE_NORMAL); > > > > + (pci_get_byte(d->config + PCI_HEADER_TYPE) & > > > > + PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL); > > > > > > what is this doing? > > > > It changes the header type to normal device(bit 1-7) without overwriting > > multi function bit(bit 8). > > Don't we know what the multi function bit value is? pci generic initialization, pci_qdev_init(), in pci.c sets (or clears) the bit and then calls the device specific initialization function, pbm_pci_host_init() in this case. So we shouldn't clear the bit unconditionally. > > Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo, > > on the other hand pbc_pci_host_init() sets the register > > to PCI_HEADER_TYPE_NORMAL. > > To be honest I don't know why it does so, but that is what Blue wants. > > BTW I think it would be prettier to have is_bridge instead of header_type > as a qdev property. Agree? The spec version 3.0 defines three header types. 0:normal device, 1:pci-to-pci bridge, 2:card bus bridge So I'd like the name a bit more generic than is_bridge. Any suggestion? > > So I touch only multi function bit(bit 8) and leave other bit (bit 1-7) > > unchanged. > > > > If you don't like this hunk, I'll drop this hunk and leave it to Blue. > > What do you think? > > Blue Swirl, could you comment on this please? > > > static PCIDeviceInfo pbm_pci_host_info = { > > .qdev.name = "pbm", > > .qdev.size = sizeof(PCIDevice), > > .init = pbm_pci_host_init, > > .header_type = PCI_HEADER_TYPE_BRIDGE, <<<<< Here > > }; > > > > -- > > yamahata >
On Wed, Jun 16, 2010 at 06:43:53PM +0900, Isaku Yamahata wrote: > On Wed, Jun 16, 2010 at 11:54:25AM +0300, Michael S. Tsirkin wrote: > > On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote: > > > On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote: > > > > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote: > > > > > Don't overwrite pci header type. > > > > > Otherwise, multi function bit which pci_init_header_type() sets > > > > > appropriately is lost. > > > > > Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary to zero > > > > > which is already zero cleared. > > > > > > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > > > > > > > ... > > > > > > > > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c > > > > > index 31c8d70..cdf3bc2 100644 > > > > > --- a/hw/apb_pci.c > > > > > +++ b/hw/apb_pci.c > > > > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d) > > > > > PCI_STATUS_DEVSEL_MEDIUM); > > > > > pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); > > > > > pci_set_byte(d->config + PCI_HEADER_TYPE, > > > > > - PCI_HEADER_TYPE_NORMAL); > > > > > + (pci_get_byte(d->config + PCI_HEADER_TYPE) & > > > > > + PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL); > > > > > > > > what is this doing? > > > > > > It changes the header type to normal device(bit 1-7) without overwriting > > > multi function bit(bit 8). > > > > Don't we know what the multi function bit value is? > > pci generic initialization, pci_qdev_init(), in pci.c sets (or clears) the bit > and then calls the device specific initialization function, pbm_pci_host_init() > in this case. > So we shouldn't clear the bit unconditionally. > > > > > Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo, > > > on the other hand pbc_pci_host_init() sets the register > > > to PCI_HEADER_TYPE_NORMAL. > > > To be honest I don't know why it does so, but that is what Blue wants. > > > > BTW I think it would be prettier to have is_bridge instead of header_type > > as a qdev property. Agree? > > The spec version 3.0 defines three header types. > 0:normal device, 1:pci-to-pci bridge, 2:card bus bridge > So I'd like the name a bit more generic than is_bridge. > Any suggestion? Could we just have functions that set up header for each type, such as pci_init_normal_header() pci_init_p2p_bridge_header() pci_init_cardbus_header() > > > So I touch only multi function bit(bit 8) and leave other bit (bit 1-7) > > > unchanged. > > > > > > If you don't like this hunk, I'll drop this hunk and leave it to Blue. > > > What do you think? > > > > Blue Swirl, could you comment on this please? > > > > > static PCIDeviceInfo pbm_pci_host_info = { > > > .qdev.name = "pbm", > > > .qdev.size = sizeof(PCIDevice), > > > .init = pbm_pci_host_init, > > > .header_type = PCI_HEADER_TYPE_BRIDGE, <<<<< Here > > > }; > > > > > > -- > > > yamahata > > > > -- > yamahata
On Wed, Jun 16, 2010 at 02:19:44PM +0300, Michael S. Tsirkin wrote: > > > > Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo, > > > > on the other hand pbc_pci_host_init() sets the register > > > > to PCI_HEADER_TYPE_NORMAL. > > > > To be honest I don't know why it does so, but that is what Blue wants. > > > > > > BTW I think it would be prettier to have is_bridge instead of header_type > > > as a qdev property. Agree? > > > > The spec version 3.0 defines three header types. > > 0:normal device, 1:pci-to-pci bridge, 2:card bus bridge > > So I'd like the name a bit more generic than is_bridge. > > Any suggestion? > > Could we just have functions that set up header for > each type, such as > pci_init_normal_header() > pci_init_p2p_bridge_header() > pci_init_cardbus_header() I see. You mean device specific initialization function should call one of them. Then header_type property will be dropped. I'll split pci p2p bridge related functions into a file at first. Then introduce helper functions.
On Wed, Jun 16, 2010 at 08:38:18PM +0900, Isaku Yamahata wrote: > On Wed, Jun 16, 2010 at 02:19:44PM +0300, Michael S. Tsirkin wrote: > > > > > Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo, > > > > > on the other hand pbc_pci_host_init() sets the register > > > > > to PCI_HEADER_TYPE_NORMAL. > > > > > To be honest I don't know why it does so, but that is what Blue wants. > > > > > > > > BTW I think it would be prettier to have is_bridge instead of header_type > > > > as a qdev property. Agree? > > > > > > The spec version 3.0 defines three header types. > > > 0:normal device, 1:pci-to-pci bridge, 2:card bus bridge > > > So I'd like the name a bit more generic than is_bridge. > > > Any suggestion? > > > > Could we just have functions that set up header for > > each type, such as > > pci_init_normal_header() > > pci_init_p2p_bridge_header() > > pci_init_cardbus_header() > > I see. You mean device specific initialization function should > call one of them. Then header_type property will be dropped. > I'll split pci p2p bridge related functions into a file > at first. > Then introduce helper functions. Just to clarify what I meant: the common pci spec implementation should be in pci.c, any platform that supports pci will need it. What I think we want to move to pc_pci_bridge.c or such is this: static PCIDeviceInfo bridge_info = { .qdev.name = "pci-bridge", .qdev.size = sizeof(PCIBridge), .init = pci_bridge_initfn, .exit = pci_bridge_exitfn, .config_write = pci_bridge_write_config, .header_type = PCI_HEADER_TYPE_BRIDGE, .qdev.props = (Property[]) { DEFINE_PROP_HEX32("vendorid", PCIBridge, vid, 0), DEFINE_PROP_HEX32("deviceid", PCIBridge, did, 0), DEFINE_PROP_END_OF_LIST(), } }; Because if I understand correctly, this is not "the bridge", it's just a pci bridge that PC has, but it is currently instanciated even on platforms where it's unused. This way we can avoid linking it on these platforms. But I think the bridge header setup is common so it should be implemented in a set of common functions and stay in pci.c, then all bridges can call these functions. > -- > yamahata
On Wed, Jun 16, 2010 at 8:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote: >> On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote: >> > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote: >> > > Don't overwrite pci header type. >> > > Otherwise, multi function bit which pci_init_header_type() sets >> > > appropriately is lost. >> > > Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary to zero >> > > which is already zero cleared. >> > > >> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> >> > >> > ... >> > >> > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c >> > > index 31c8d70..cdf3bc2 100644 >> > > --- a/hw/apb_pci.c >> > > +++ b/hw/apb_pci.c >> > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d) >> > > PCI_STATUS_DEVSEL_MEDIUM); >> > > pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); >> > > pci_set_byte(d->config + PCI_HEADER_TYPE, >> > > - PCI_HEADER_TYPE_NORMAL); >> > > + (pci_get_byte(d->config + PCI_HEADER_TYPE) & >> > > + PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL); >> > >> > what is this doing? >> >> It changes the header type to normal device(bit 1-7) without overwriting >> multi function bit(bit 8). > > Don't we know what the multi function bit value is? > >> Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo, >> on the other hand pbc_pci_host_init() sets the register >> to PCI_HEADER_TYPE_NORMAL. >> To be honest I don't know why it does so, but that is what Blue wants. > > BTW I think it would be prettier to have is_bridge instead of header_type > as a qdev property. Agree? Good idea. >> So I touch only multi function bit(bit 8) and leave other bit (bit 1-7) >> unchanged. >> >> If you don't like this hunk, I'll drop this hunk and leave it to Blue. >> What do you think? > > Blue Swirl, could you comment on this please? I'd go for is_bridge and drop the override for header type in apb_pci.c then. >> static PCIDeviceInfo pbm_pci_host_info = { >> .qdev.name = "pbm", >> .qdev.size = sizeof(PCIDevice), >> .init = pbm_pci_host_init, >> .header_type = PCI_HEADER_TYPE_BRIDGE, <<<<< Here >> }; >> >> -- >> yamahata >
On Wed, Jun 16, 2010 at 06:41:22PM +0000, Blue Swirl wrote: > On Wed, Jun 16, 2010 at 8:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote: > >> On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote: > >> > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote: > >> > > Don't overwrite pci header type. > >> > > Otherwise, multi function bit which pci_init_header_type() sets > >> > > appropriately is lost. > >> > > Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary to zero > >> > > which is already zero cleared. > >> > > > >> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > >> > > >> > ... > >> > > >> > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c > >> > > index 31c8d70..cdf3bc2 100644 > >> > > --- a/hw/apb_pci.c > >> > > +++ b/hw/apb_pci.c > >> > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d) > >> > > PCI_STATUS_DEVSEL_MEDIUM); > >> > > pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); > >> > > pci_set_byte(d->config + PCI_HEADER_TYPE, > >> > > - PCI_HEADER_TYPE_NORMAL); > >> > > + (pci_get_byte(d->config + PCI_HEADER_TYPE) & > >> > > + PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL); > >> > > >> > what is this doing? > >> > >> It changes the header type to normal device(bit 1-7) without overwriting > >> multi function bit(bit 8). > > > > Don't we know what the multi function bit value is? > > > >> Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo, > >> on the other hand pbc_pci_host_init() sets the register > >> to PCI_HEADER_TYPE_NORMAL. > >> To be honest I don't know why it does so, but that is what Blue wants. > > > > BTW I think it would be prettier to have is_bridge instead of header_type > > as a qdev property. Agree? > > Good idea. > > >> So I touch only multi function bit(bit 8) and leave other bit (bit 1-7) > >> unchanged. > >> > >> If you don't like this hunk, I'll drop this hunk and leave it to Blue. > >> What do you think? > > > > Blue Swirl, could you comment on this please? > > I'd go for is_bridge and drop the override for header type in apb_pci.c then. Yes, but what header type does it need? > >> static PCIDeviceInfo pbm_pci_host_info = { > >> .qdev.name = "pbm", > >> .qdev.size = sizeof(PCIDevice), > >> .init = pbm_pci_host_init, > >> .header_type = PCI_HEADER_TYPE_BRIDGE, <<<<< Here > >> }; > >> > >> -- > >> yamahata > >
On Wed, Jun 16, 2010 at 6:51 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Jun 16, 2010 at 06:41:22PM +0000, Blue Swirl wrote: >> On Wed, Jun 16, 2010 at 8:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote: >> >> On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote: >> >> > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote: >> >> > > Don't overwrite pci header type. >> >> > > Otherwise, multi function bit which pci_init_header_type() sets >> >> > > appropriately is lost. >> >> > > Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary to zero >> >> > > which is already zero cleared. >> >> > > >> >> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> >> >> > >> >> > ... >> >> > >> >> > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c >> >> > > index 31c8d70..cdf3bc2 100644 >> >> > > --- a/hw/apb_pci.c >> >> > > +++ b/hw/apb_pci.c >> >> > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d) >> >> > > PCI_STATUS_DEVSEL_MEDIUM); >> >> > > pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); >> >> > > pci_set_byte(d->config + PCI_HEADER_TYPE, >> >> > > - PCI_HEADER_TYPE_NORMAL); >> >> > > + (pci_get_byte(d->config + PCI_HEADER_TYPE) & >> >> > > + PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL); >> >> > >> >> > what is this doing? >> >> >> >> It changes the header type to normal device(bit 1-7) without overwriting >> >> multi function bit(bit 8). >> > >> > Don't we know what the multi function bit value is? >> > >> >> Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo, >> >> on the other hand pbc_pci_host_init() sets the register >> >> to PCI_HEADER_TYPE_NORMAL. >> >> To be honest I don't know why it does so, but that is what Blue wants. >> > >> > BTW I think it would be prettier to have is_bridge instead of header_type >> > as a qdev property. Agree? >> >> Good idea. >> >> >> So I touch only multi function bit(bit 8) and leave other bit (bit 1-7) >> >> unchanged. >> >> >> >> If you don't like this hunk, I'll drop this hunk and leave it to Blue. >> >> What do you think? >> > >> > Blue Swirl, could you comment on this please? >> >> I'd go for is_bridge and drop the override for header type in apb_pci.c then. > > Yes, but what header type does it need? The type should be bridge (to allow writes to bridge registers), but PCI header should use PCI_HEADER_TYPE_NORMAL (because the PBM specification says so). >> >> static PCIDeviceInfo pbm_pci_host_info = { >> >> .qdev.name = "pbm", >> >> .qdev.size = sizeof(PCIDevice), >> >> .init = pbm_pci_host_init, >> >> .header_type = PCI_HEADER_TYPE_BRIDGE, <<<<< Here >> >> }; >> >> >> >> -- >> >> yamahata >> > >
On Wed, Jun 16, 2010 at 07:02:54PM +0000, Blue Swirl wrote: > On Wed, Jun 16, 2010 at 6:51 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Jun 16, 2010 at 06:41:22PM +0000, Blue Swirl wrote: > >> On Wed, Jun 16, 2010 at 8:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > >> > On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote: > >> >> On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote: > >> >> > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote: > >> >> > > Don't overwrite pci header type. > >> >> > > Otherwise, multi function bit which pci_init_header_type() sets > >> >> > > appropriately is lost. > >> >> > > Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary to zero > >> >> > > which is already zero cleared. > >> >> > > > >> >> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > >> >> > > >> >> > ... > >> >> > > >> >> > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c > >> >> > > index 31c8d70..cdf3bc2 100644 > >> >> > > --- a/hw/apb_pci.c > >> >> > > +++ b/hw/apb_pci.c > >> >> > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d) > >> >> > > PCI_STATUS_DEVSEL_MEDIUM); > >> >> > > pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); > >> >> > > pci_set_byte(d->config + PCI_HEADER_TYPE, > >> >> > > - PCI_HEADER_TYPE_NORMAL); > >> >> > > + (pci_get_byte(d->config + PCI_HEADER_TYPE) & > >> >> > > + PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL); > >> >> > > >> >> > what is this doing? > >> >> > >> >> It changes the header type to normal device(bit 1-7) without overwriting > >> >> multi function bit(bit 8). > >> > > >> > Don't we know what the multi function bit value is? > >> > > >> >> Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo, > >> >> on the other hand pbc_pci_host_init() sets the register > >> >> to PCI_HEADER_TYPE_NORMAL. > >> >> To be honest I don't know why it does so, but that is what Blue wants. > >> > > >> > BTW I think it would be prettier to have is_bridge instead of header_type > >> > as a qdev property. Agree? > >> > >> Good idea. > >> > >> >> So I touch only multi function bit(bit 8) and leave other bit (bit 1-7) > >> >> unchanged. > >> >> > >> >> If you don't like this hunk, I'll drop this hunk and leave it to Blue. > >> >> What do you think? > >> > > >> > Blue Swirl, could you comment on this please? > >> > >> I'd go for is_bridge and drop the override for header type in apb_pci.c then. > > > > Yes, but what header type does it need? > > The type should be bridge (to allow writes to bridge registers), but > PCI header should use PCI_HEADER_TYPE_NORMAL (because the PBM > specification says so). I can no longer get the PBM specs now: are there alternative links? Need to fix links in code. > >> >> static PCIDeviceInfo pbm_pci_host_info = { > >> >> .qdev.name = "pbm", > >> >> .qdev.size = sizeof(PCIDevice), > >> >> .init = pbm_pci_host_init, > >> >> .header_type = PCI_HEADER_TYPE_BRIDGE, <<<<< Here > >> >> }; > >> >> > >> >> -- > >> >> yamahata > >> > > >
On Wed, Jun 16, 2010 at 7:22 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Jun 16, 2010 at 07:02:54PM +0000, Blue Swirl wrote: >> On Wed, Jun 16, 2010 at 6:51 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > On Wed, Jun 16, 2010 at 06:41:22PM +0000, Blue Swirl wrote: >> >> On Wed, Jun 16, 2010 at 8:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> > On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote: >> >> >> On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote: >> >> >> > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote: >> >> >> > > Don't overwrite pci header type. >> >> >> > > Otherwise, multi function bit which pci_init_header_type() sets >> >> >> > > appropriately is lost. >> >> >> > > Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary to zero >> >> >> > > which is already zero cleared. >> >> >> > > >> >> >> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> >> >> >> > >> >> >> > ... >> >> >> > >> >> >> > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c >> >> >> > > index 31c8d70..cdf3bc2 100644 >> >> >> > > --- a/hw/apb_pci.c >> >> >> > > +++ b/hw/apb_pci.c >> >> >> > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d) >> >> >> > > PCI_STATUS_DEVSEL_MEDIUM); >> >> >> > > pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); >> >> >> > > pci_set_byte(d->config + PCI_HEADER_TYPE, >> >> >> > > - PCI_HEADER_TYPE_NORMAL); >> >> >> > > + (pci_get_byte(d->config + PCI_HEADER_TYPE) & >> >> >> > > + PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL); >> >> >> > >> >> >> > what is this doing? >> >> >> >> >> >> It changes the header type to normal device(bit 1-7) without overwriting >> >> >> multi function bit(bit 8). >> >> > >> >> > Don't we know what the multi function bit value is? >> >> > >> >> >> Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo, >> >> >> on the other hand pbc_pci_host_init() sets the register >> >> >> to PCI_HEADER_TYPE_NORMAL. >> >> >> To be honest I don't know why it does so, but that is what Blue wants. >> >> > >> >> > BTW I think it would be prettier to have is_bridge instead of header_type >> >> > as a qdev property. Agree? >> >> >> >> Good idea. >> >> >> >> >> So I touch only multi function bit(bit 8) and leave other bit (bit 1-7) >> >> >> unchanged. >> >> >> >> >> >> If you don't like this hunk, I'll drop this hunk and leave it to Blue. >> >> >> What do you think? >> >> > >> >> > Blue Swirl, could you comment on this please? >> >> >> >> I'd go for is_bridge and drop the override for header type in apb_pci.c then. >> > >> > Yes, but what header type does it need? >> >> The type should be bridge (to allow writes to bridge registers), but >> PCI header should use PCI_HEADER_TYPE_NORMAL (because the PBM >> specification says so). > > I can no longer get the PBM specs now: are there > alternative links? Need to fix links in code. That sucks. I hope this is only temporary. > > >> >> >> static PCIDeviceInfo pbm_pci_host_info = { >> >> >> .qdev.name = "pbm", >> >> >> .qdev.size = sizeof(PCIDevice), >> >> >> .init = pbm_pci_host_init, >> >> >> .header_type = PCI_HEADER_TYPE_BRIDGE, <<<<< Here >> >> >> }; >> >> >> >> >> >> -- >> >> >> yamahata >> >> > >> > >
On 06/16/2010 02:22 PM, Michael S. Tsirkin wrote: > On Wed, Jun 16, 2010 at 07:02:54PM +0000, Blue Swirl wrote: > >> On Wed, Jun 16, 2010 at 6:51 PM, Michael S. Tsirkin<mst@redhat.com> wrote: >> >>> On Wed, Jun 16, 2010 at 06:41:22PM +0000, Blue Swirl wrote: >>> >>>> On Wed, Jun 16, 2010 at 8:54 AM, Michael S. Tsirkin<mst@redhat.com> wrote: >>>> >>>>> On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote: >>>>> >>>>>> On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrote: >>>>>> >>>>>>> On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote: >>>>>>> >>>>>>>> Don't overwrite pci header type. >>>>>>>> Otherwise, multi function bit which pci_init_header_type() sets >>>>>>>> appropriately is lost. >>>>>>>> Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary to zero >>>>>>>> which is already zero cleared. >>>>>>>> >>>>>>>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> >>>>>>>> >>>>>>> ... >>>>>>> >>>>>>> >>>>>>>> diff --git a/hw/apb_pci.c b/hw/apb_pci.c >>>>>>>> index 31c8d70..cdf3bc2 100644 >>>>>>>> --- a/hw/apb_pci.c >>>>>>>> +++ b/hw/apb_pci.c >>>>>>>> @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d) >>>>>>>> PCI_STATUS_DEVSEL_MEDIUM); >>>>>>>> pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); >>>>>>>> pci_set_byte(d->config + PCI_HEADER_TYPE, >>>>>>>> - PCI_HEADER_TYPE_NORMAL); >>>>>>>> + (pci_get_byte(d->config + PCI_HEADER_TYPE)& >>>>>>>> + PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL); >>>>>>>> >>>>>>> what is this doing? >>>>>>> >>>>>> It changes the header type to normal device(bit 1-7) without overwriting >>>>>> multi function bit(bit 8). >>>>>> >>>>> Don't we know what the multi function bit value is? >>>>> >>>>> >>>>>> Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo, >>>>>> on the other hand pbc_pci_host_init() sets the register >>>>>> to PCI_HEADER_TYPE_NORMAL. >>>>>> To be honest I don't know why it does so, but that is what Blue wants. >>>>>> >>>>> BTW I think it would be prettier to have is_bridge instead of header_type >>>>> as a qdev property. Agree? >>>>> >>>> Good idea. >>>> >>>> >>>>>> So I touch only multi function bit(bit 8) and leave other bit (bit 1-7) >>>>>> unchanged. >>>>>> >>>>>> If you don't like this hunk, I'll drop this hunk and leave it to Blue. >>>>>> What do you think? >>>>>> >>>>> Blue Swirl, could you comment on this please? >>>>> >>>> I'd go for is_bridge and drop the override for header type in apb_pci.c then. >>>> >>> Yes, but what header type does it need? >>> >> The type should be bridge (to allow writes to bridge registers), but >> PCI header should use PCI_HEADER_TYPE_NORMAL (because the PBM >> specification says so). >> > I can no longer get the PBM specs now: are there > alternative links? Need to fix links in code. > BTW, I set up http://wiki.qemu.org/Documentation/HardwareManuals so we could start archiving these specification when allowed. Regards, Anthony Liguori > >>>>>> static PCIDeviceInfo pbm_pci_host_info = { >>>>>> .qdev.name = "pbm", >>>>>> .qdev.size = sizeof(PCIDevice), >>>>>> .init = pbm_pci_host_init, >>>>>> .header_type = PCI_HEADER_TYPE_BRIDGE,<<<<< Here >>>>>> }; >>>>>> >>>>>> -- >>>>>> yamahata >>>>>> >>>>> >>> >
diff --git a/hw/ac97.c b/hw/ac97.c index 4319bc8..d71072d 100644 --- a/hw/ac97.c +++ b/hw/ac97.c @@ -1295,7 +1295,6 @@ static int ac97_initfn (PCIDevice *dev) c[PCI_REVISION_ID] = 0x01; /* rid revision ro */ c[PCI_CLASS_PROG] = 0x00; /* pi programming interface ro */ pci_config_set_class (c, PCI_CLASS_MULTIMEDIA_AUDIO); /* ro */ - c[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; /* headtyp header type ro */ /* TODO set when bar is registered. no need to override. */ /* nabmar native audio mixer base address rw */ diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c index 8d1a628..bfa1d9a 100644 --- a/hw/acpi_piix4.c +++ b/hw/acpi_piix4.c @@ -369,7 +369,6 @@ static int piix4_pm_initfn(PCIDevice *dev) pci_conf[0x08] = 0x03; // revision number pci_conf[0x09] = 0x00; pci_config_set_class(pci_conf, PCI_CLASS_BRIDGE_OTHER); - pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type pci_conf[0x3d] = 0x01; // interrupt pin 1 pci_conf[0x40] = 0x01; /* PM io base read only bit */ diff --git a/hw/apb_pci.c b/hw/apb_pci.c index 31c8d70..cdf3bc2 100644 --- a/hw/apb_pci.c +++ b/hw/apb_pci.c @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d) PCI_STATUS_DEVSEL_MEDIUM); pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); pci_set_byte(d->config + PCI_HEADER_TYPE, - PCI_HEADER_TYPE_NORMAL); + (pci_get_byte(d->config + PCI_HEADER_TYPE) & + PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL); return 0; } diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c index aa0c51b..b3a5f54 100644 --- a/hw/grackle_pci.c +++ b/hw/grackle_pci.c @@ -126,7 +126,6 @@ static int grackle_pci_host_init(PCIDevice *d) d->config[0x08] = 0x00; // revision d->config[0x09] = 0x01; pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); - d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type return 0; } diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c index 559147f..756ee81 100644 --- a/hw/ide/cmd646.c +++ b/hw/ide/cmd646.c @@ -240,7 +240,6 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev) pci_conf[PCI_CLASS_PROG] = 0x8f; pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_IDE); - pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type pci_conf[0x51] = 0x04; // enable IDE0 if (d->secondary) { diff --git a/hw/ide/piix.c b/hw/ide/piix.c index dad6e86..8817915 100644 --- a/hw/ide/piix.c +++ b/hw/ide/piix.c @@ -122,7 +122,6 @@ static int pci_piix_ide_initfn(PCIIDEState *d) pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_IDE); - pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type qemu_register_reset(piix3_reset, d); diff --git a/hw/macio.c b/hw/macio.c index e92e82a..789ca55 100644 --- a/hw/macio.c +++ b/hw/macio.c @@ -110,7 +110,6 @@ void macio_init (PCIBus *bus, int device_id, int is_oldworld, int pic_mem_index, pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_APPLE); pci_config_set_device_id(d->config, device_id); pci_config_set_class(d->config, PCI_CLASS_OTHERS << 8); - d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type d->config[0x3d] = 0x01; // interrupt on pin 1 diff --git a/hw/ne2000.c b/hw/ne2000.c index 78fe14f..126e7cf 100644 --- a/hw/ne2000.c +++ b/hw/ne2000.c @@ -723,7 +723,6 @@ static int pci_ne2000_init(PCIDevice *pci_dev) pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_REALTEK); pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_REALTEK_8029); pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET); - pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type /* TODO: RST# value should be 0. PCI spec 6.2.4 */ pci_conf[PCI_INTERRUPT_PIN] = 1; // interrupt pin 0 diff --git a/hw/openpic.c b/hw/openpic.c index ac21993..2bbf787 100644 --- a/hw/openpic.c +++ b/hw/openpic.c @@ -1194,7 +1194,6 @@ qemu_irq *openpic_init (PCIBus *bus, int *pmem_index, int nb_cpus, pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_IBM); pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_IBM_OPENPIC2); pci_config_set_class(pci_conf, PCI_CLASS_SYSTEM_OTHER); // FIXME? - pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type pci_conf[0x3d] = 0x00; // no interrupt pin /* Register I/O spaces */ diff --git a/hw/pcnet.c b/hw/pcnet.c index 5e63eb5..5e75930 100644 --- a/hw/pcnet.c +++ b/hw/pcnet.c @@ -1990,7 +1990,6 @@ static int pci_pcnet_init(PCIDevice *pci_dev) /* TODO: 0 is the default anyway, no need to set it. */ pci_conf[PCI_CLASS_PROG] = 0x00; pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET); - pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type /* TODO: not necessary, is set when BAR is registered. */ pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_SPACE_IO); diff --git a/hw/piix4.c b/hw/piix4.c index f75951b..03926a7 100644 --- a/hw/piix4.c +++ b/hw/piix4.c @@ -93,8 +93,7 @@ static int piix4_initfn(PCIDevice *d) pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL); pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82371AB_0); // 82371AB/EB/MB PIIX4 PCI-to-ISA bridge pci_config_set_class(pci_conf, PCI_CLASS_BRIDGE_ISA); - pci_conf[PCI_HEADER_TYPE] = - PCI_HEADER_TYPE_NORMAL | PCI_HEADER_TYPE_MULTI_FUNCTION; // header_type = PCI_multifunction, generic + pci_conf[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION; piix4_dev = d; qemu_register_reset(piix4_reset, d); diff --git a/hw/piix_pci.c b/hw/piix_pci.c index d14d05e..51e8c46 100644 --- a/hw/piix_pci.c +++ b/hw/piix_pci.c @@ -208,7 +208,6 @@ static int i440fx_initfn(PCIDevice *dev) pci_config_set_device_id(d->dev.config, PCI_DEVICE_ID_INTEL_82441); d->dev.config[0x08] = 0x02; // revision pci_config_set_class(d->dev.config, PCI_CLASS_BRIDGE_HOST); - d->dev.config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type d->dev.config[I440FX_SMRAM] = 0x02; @@ -336,8 +335,7 @@ static int piix3_initfn(PCIDevice *dev) pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL); pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82371SB_0); // 82371SB PIIX3 PCI-to-ISA bridge (Step A1) pci_config_set_class(pci_conf, PCI_CLASS_BRIDGE_ISA); - pci_conf[PCI_HEADER_TYPE] = - PCI_HEADER_TYPE_NORMAL | PCI_HEADER_TYPE_MULTI_FUNCTION; // header_type = PCI_multifunction, generic + pci_conf[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION; qemu_register_reset(piix3_reset, d); return 0; diff --git a/hw/prep_pci.c b/hw/prep_pci.c index 144fde0..0c2afe9 100644 --- a/hw/prep_pci.c +++ b/hw/prep_pci.c @@ -137,7 +137,6 @@ PCIBus *pci_prep_init(qemu_irq *pic) pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); d->config[0x0C] = 0x08; // cache_line_size d->config[0x0D] = 0x10; // latency_timer - d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type d->config[0x34] = 0x00; // capabilities_pointer return s->bus; diff --git a/hw/rtl8139.c b/hw/rtl8139.c index 72e2242..441f0a9 100644 --- a/hw/rtl8139.c +++ b/hw/rtl8139.c @@ -3361,7 +3361,6 @@ static int pci_rtl8139_init(PCIDevice *dev) pci_conf[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MASTER; pci_conf[PCI_REVISION_ID] = RTL8139_PCI_REVID; /* >=0x20 is for 8139C+ */ pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET); - pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; /* TODO: value should be 0 at RST# */ pci_conf[PCI_INTERRUPT_PIN] = 1; /* interrupt pin 0 */ /* TODO: start of capability list, but no capability diff --git a/hw/sun4u.c b/hw/sun4u.c index 40b5f1f..cf5a8c4 100644 --- a/hw/sun4u.c +++ b/hw/sun4u.c @@ -562,7 +562,6 @@ pci_ebus_init1(PCIDevice *s) s->config[0x09] = 0x00; // programming i/f pci_config_set_class(s->config, PCI_CLASS_BRIDGE_OTHER); s->config[0x0D] = 0x0a; // latency_timer - s->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type pci_register_bar(s, 0, 0x1000000, PCI_BASE_ADDRESS_SPACE_MEMORY, ebus_mmio_mapfunc); diff --git a/hw/unin_pci.c b/hw/unin_pci.c index f0a773d..7b1c94b 100644 --- a/hw/unin_pci.c +++ b/hw/unin_pci.c @@ -298,7 +298,6 @@ static int unin_main_pci_host_init(PCIDevice *d) pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); d->config[0x0C] = 0x08; // cache_line_size d->config[0x0D] = 0x10; // latency_timer - d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type d->config[0x34] = 0x00; // capabilities_pointer return 0; } @@ -311,7 +310,6 @@ static int unin_agp_pci_host_init(PCIDevice *d) pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); d->config[0x0C] = 0x08; // cache_line_size d->config[0x0D] = 0x10; // latency_timer - d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type // d->config[0x34] = 0x80; // capabilities_pointer return 0; } @@ -327,7 +325,6 @@ static int u3_agp_pci_host_init(PCIDevice *d) d->config[0x0C] = 0x08; /* latency timer */ d->config[0x0D] = 0x10; - d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; return 0; } @@ -339,7 +336,6 @@ static int unin_internal_pci_host_init(PCIDevice *d) pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); d->config[0x0C] = 0x08; // cache_line_size d->config[0x0D] = 0x10; // latency_timer - d->config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type d->config[0x34] = 0x00; // capabilities_pointer return 0; } diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c index 624d55b..058bf59 100644 --- a/hw/usb-uhci.c +++ b/hw/usb-uhci.c @@ -1108,7 +1108,6 @@ static int usb_uhci_common_initfn(UHCIState *s) pci_conf[PCI_REVISION_ID] = 0x01; // revision number pci_conf[PCI_CLASS_PROG] = 0x00; pci_config_set_class(pci_conf, PCI_CLASS_SERIAL_USB); - pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type /* TODO: reset value should be 0. */ pci_conf[PCI_INTERRUPT_PIN] = 4; // interrupt pin 3 pci_conf[0x60] = 0x10; // release number diff --git a/hw/vga-pci.c b/hw/vga-pci.c index eef78ed..2315f70 100644 --- a/hw/vga-pci.c +++ b/hw/vga-pci.c @@ -90,7 +90,6 @@ static int pci_vga_initfn(PCIDevice *dev) pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_QEMU); pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_QEMU_VGA); pci_config_set_class(pci_conf, PCI_CLASS_DISPLAY_VGA); - pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type /* XXX: VGA_RAM_SIZE must be a power of two */ pci_register_bar(&d->dev, 0, VGA_RAM_SIZE, diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index e101fa0..0e25f25 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -506,7 +506,6 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev, config[0x09] = pif; pci_config_set_class(config, class_code); - config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; config[0x2c] = vendor & 0xFF; config[0x2d] = (vendor >> 8) & 0xFF; diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c index bf2a699..38fe976 100644 --- a/hw/vmware_vga.c +++ b/hw/vmware_vga.c @@ -1246,7 +1246,6 @@ static int pci_vmsvga_initfn(PCIDevice *dev) pci_config_set_class(s->card.config, PCI_CLASS_DISPLAY_VGA); s->card.config[PCI_CACHE_LINE_SIZE] = 0x08; /* Cache line size */ s->card.config[PCI_LATENCY_TIMER] = 0x40; /* Latency timer */ - s->card.config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; s->card.config[PCI_SUBSYSTEM_VENDOR_ID] = PCI_VENDOR_ID_VMWARE & 0xff; s->card.config[PCI_SUBSYSTEM_VENDOR_ID + 1] = PCI_VENDOR_ID_VMWARE >> 8; s->card.config[PCI_SUBSYSTEM_ID] = SVGA_PCI_DEVICE_ID & 0xff; diff --git a/hw/wdt_i6300esb.c b/hw/wdt_i6300esb.c index be0e89e..46e1df8 100644 --- a/hw/wdt_i6300esb.c +++ b/hw/wdt_i6300esb.c @@ -411,7 +411,6 @@ static int i6300esb_init(PCIDevice *dev) pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL); pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_ESB_9); pci_config_set_class(pci_conf, PCI_CLASS_SYSTEM_OTHER); - pci_conf[PCI_HEADER_TYPE] = 0x00; pci_register_bar(&d->dev, 0, 0x10, PCI_BASE_ADDRESS_SPACE_MEMORY, i6300esb_map);
Don't overwrite pci header type. Otherwise, multi function bit which pci_init_header_type() sets appropriately is lost. Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary to zero which is already zero cleared. Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/ac97.c | 1 - hw/acpi_piix4.c | 1 - hw/apb_pci.c | 3 ++- hw/grackle_pci.c | 1 - hw/ide/cmd646.c | 1 - hw/ide/piix.c | 1 - hw/macio.c | 1 - hw/ne2000.c | 1 - hw/openpic.c | 1 - hw/pcnet.c | 1 - hw/piix4.c | 3 +-- hw/piix_pci.c | 4 +--- hw/prep_pci.c | 1 - hw/rtl8139.c | 1 - hw/sun4u.c | 1 - hw/unin_pci.c | 4 ---- hw/usb-uhci.c | 1 - hw/vga-pci.c | 1 - hw/virtio-pci.c | 1 - hw/vmware_vga.c | 1 - hw/wdt_i6300esb.c | 1 - 21 files changed, 4 insertions(+), 27 deletions(-)