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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Oct 13, 2009 at 10:20:07PM +0900, Isaku Yamahata wrote:
> 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 <yamahata@valinux.co.jp>
> > > ---
> > >  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?

That'll work, but let's start with Cc relevant people.  The issue here is
that the bridge is inited with memory enabled which is out of spec, but
we are concerned what fixing this will do - in the past such fixes
sometimes triggered BIOS bugs.  Anyone can test a fix for this?

> 
> 
> > > +    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