[Qemu-devel] Re: [PATCH V5 07/29] pci/bridge: clean up of pci_bridge_initfn()

[Isaku Yamahata]

On Fri, Oct 09, 2009 at 08:53:10AM +0200, Michael S. Tsirkin wrote:
> On Fri, Oct 09, 2009 at 03:28:40PM +0900, Isaku Yamahata wrote:
> > - use symbolic constant
> > - use helper function pci_set_xxx()
> > 
> > Signed-off-by: Isaku Yamahata <address@hidden>
> > ---
> >  hw/pci.c |   23 ++++++++++++-----------
> >  1 files changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index a66e3de..eaf471a 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -923,17 +923,18 @@ static int pci_bridge_initfn(PCIDevice *dev)
> >      pci_config_set_vendor_id(s->dev.config, s->vid);
> >      pci_config_set_device_id(s->dev.config, s->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_set_word(dev->config + PCI_COMMAND,
> > +                 PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> 
> BTW, I think this is a wrong reset value: should be disabled
> by default. Fixing this needs some testing though, so
> I am not suggesting we do it in this patch. Have some time
> to fix this?

Hmm, the user of it is only apb_pci.c
I guess other magic values came from the real machine.
So one possible fix is to create apb_pci specific initialization function
and to move those initialization code into apb_pci.c leaving to
sparc guys. So we can avoid breakage.
What do you think of it?



> > +    pci_set_word(dev->config + PCI_STATUS,
> > +                 PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
> > +    dev->config[PCI_REVISION_ID] = 0x00;
> > +    dev->config[PCI_CLASS_PROG] = 0x00;
> 
> I think we can remove these two: this is the default already.

Okay.


> > +    pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_PCI);
> > +    dev->config[PCI_LATENCY_TIMER] = 0x10;
> > +    dev->config[PCI_HEADER_TYPE] =
> > +        PCI_HEADER_TYPE_MULTI_FUNCTION | PCI_HEADER_TYPE_BRIDGE;
> > +    pci_set_word(dev->config + PCI_SEC_STATUS,
> > +                 PCI_STATUS_66MHZ | PCI_STATUS_FAST_BACK);
> >      return 0;
> >  }
> >  
> 

-- 
yamahata