From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MxiE9-0007sy-8c for qemu-devel@nongnu.org; Tue, 13 Oct 2009 10:19:49 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MxiE4-0007pz-L0 for qemu-devel@nongnu.org; Tue, 13 Oct 2009 10:19:48 -0400 Received: from [199.232.76.173] (port=47345 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MxiE4-0007pq-Gm for qemu-devel@nongnu.org; Tue, 13 Oct 2009 10:19:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:5439) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MxiE4-0001kn-1u for qemu-devel@nongnu.org; Tue, 13 Oct 2009 10:19:44 -0400 Date: Tue, 13 Oct 2009 16:17:32 +0200 From: "Michael S. Tsirkin" Message-ID: <20091013141732.GA17895@redhat.com> References: <1255069742-15724-1-git-send-email-yamahata@valinux.co.jp> <1255069742-15724-8-git-send-email-yamahata@valinux.co.jp> <20091009065310.GD9942@redhat.com> <20091013132007.GC2306%yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20091013132007.GC2306%yamahata@valinux.co.jp> Subject: [Qemu-devel] Re: [PATCH V5 07/29] pci/bridge: clean up of pci_bridge_initfn() List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata , blauwirbel@gmail.com Cc: qemu-devel@nongnu.org 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 > > > --- > > > 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