From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MxjIa-0000gq-17 for qemu-devel@nongnu.org; Tue, 13 Oct 2009 11:28:28 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MxjIU-0000ds-Kq for qemu-devel@nongnu.org; Tue, 13 Oct 2009 11:28:26 -0400 Received: from [199.232.76.173] (port=60298 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MxjIU-0000dg-GX for qemu-devel@nongnu.org; Tue, 13 Oct 2009 11:28:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:23547) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MxjIT-0004dc-QQ for qemu-devel@nongnu.org; Tue, 13 Oct 2009 11:28:22 -0400 Date: Tue, 13 Oct 2009 17:26:10 +0200 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] Re: [PATCH V5 07/29] pci/bridge: clean up of pci_bridge_initfn() Message-ID: <20091013152610.GA20492@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=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: Isaku Yamahata , qemu-devel@nongnu.org On Tue, Oct 13, 2009 at 06:12:17PM +0300, Blue Swirl wrote: > On Tue, Oct 13, 2009 at 4:20 PM, 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 > >> > --- > >> > =A0hw/pci.c | =A0 23 ++++++++++++----------- > >> > =A01 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) > >> > =A0 =A0 =A0pci_config_set_vendor_id(s->dev.config, s->vid); > >> > =A0 =A0 =A0pci_config_set_device_id(s->dev.config, s->did); > >> > > >> > - =A0 =A0s->dev.config[0x04] =3D 0x06; // command =3D bus master, = pci mem > >> > - =A0 =A0s->dev.config[0x05] =3D 0x00; > >> > - =A0 =A0s->dev.config[0x06] =3D 0xa0; // status =3D fast back-to-= back, 66MHz, no error > >> > - =A0 =A0s->dev.config[0x07] =3D 0x00; // status =3D fast devsel > >> > - =A0 =A0s->dev.config[0x08] =3D 0x00; // revision > >> > - =A0 =A0s->dev.config[0x09] =3D 0x00; // programming i/f > >> > - =A0 =A0pci_config_set_class(s->dev.config, PCI_CLASS_BRIDGE_PCI)= ; > >> > - =A0 =A0s->dev.config[0x0D] =3D 0x10; // latency_timer > >> > - =A0 =A0s->dev.config[PCI_HEADER_TYPE] =3D > >> > - =A0 =A0 =A0 =A0PCI_HEADER_TYPE_MULTI_FUNCTION | PCI_HEADER_TYPE_= BRIDGE; // header_type > >> > - =A0 =A0s->dev.config[0x1E] =3D 0xa0; // secondary status > >> > + =A0 =A0pci_set_word(dev->config + PCI_COMMAND, > >> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 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 func= tion > > 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? >=20 > Current code is buggy. According to manual (805-1251) Link? Maybe we should put spec links in comments in relevant code ... > the reset value > should be zero, unless the boot pin is tied high (which is true) and > thus it should be PCI_COMMAND_MEMORY. >=20 > I think the default value should be zero and this should be overridden > later in apb_pci.c. Makes sense. --=20 MST