From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mxj3M-00012O-8y for qemu-devel@nongnu.org; Tue, 13 Oct 2009 11:12:44 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mxj3H-0000zm-C8 for qemu-devel@nongnu.org; Tue, 13 Oct 2009 11:12:43 -0400 Received: from [199.232.76.173] (port=59090 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mxj3H-0000zj-42 for qemu-devel@nongnu.org; Tue, 13 Oct 2009 11:12:39 -0400 Received: from mail-fx0-f214.google.com ([209.85.220.214]:65031) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Mxj3G-00025j-AD for qemu-devel@nongnu.org; Tue, 13 Oct 2009 11:12:38 -0400 Received: by fxm10 with SMTP id 10so9157400fxm.8 for ; Tue, 13 Oct 2009 08:12:37 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20091013132007.GC2306%yamahata@valinux.co.jp> 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> From: Blue Swirl Date: Tue, 13 Oct 2009 18:12:17 +0300 Message-ID: Subject: Re: [Qemu-devel] Re: [PATCH V5 07/29] pci/bridge: clean up of pci_bridge_initfn() Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" On Tue, Oct 13, 2009 at 4:20 PM, Isaku Yamahata wr= ote: > 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 >> > --- >> > =C2=A0hw/pci.c | =C2=A0 23 ++++++++++++----------- >> > =C2=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) >> > =C2=A0 =C2=A0 =C2=A0pci_config_set_vendor_id(s->dev.config, s->vid); >> > =C2=A0 =C2=A0 =C2=A0pci_config_set_device_id(s->dev.config, s->did); >> > >> > - =C2=A0 =C2=A0s->dev.config[0x04] =3D 0x06; // command =3D bus master= , pci mem >> > - =C2=A0 =C2=A0s->dev.config[0x05] =3D 0x00; >> > - =C2=A0 =C2=A0s->dev.config[0x06] =3D 0xa0; // status =3D fast back-t= o-back, 66MHz, no error >> > - =C2=A0 =C2=A0s->dev.config[0x07] =3D 0x00; // status =3D fast devsel >> > - =C2=A0 =C2=A0s->dev.config[0x08] =3D 0x00; // revision >> > - =C2=A0 =C2=A0s->dev.config[0x09] =3D 0x00; // programming i/f >> > - =C2=A0 =C2=A0pci_config_set_class(s->dev.config, PCI_CLASS_BRIDGE_PC= I); >> > - =C2=A0 =C2=A0s->dev.config[0x0D] =3D 0x10; // latency_timer >> > - =C2=A0 =C2=A0s->dev.config[PCI_HEADER_TYPE] =3D >> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0PCI_HEADER_TYPE_MULTI_FUNCTION | PCI_HEAD= ER_TYPE_BRIDGE; // header_type >> > - =C2=A0 =C2=A0s->dev.config[0x1E] =3D 0xa0; // secondary status >> > + =C2=A0 =C2=A0pci_set_word(dev->config + PCI_COMMAND, >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=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 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? Current code is buggy. According to manual (805-1251) the reset value should be zero, unless the boot pin is tied high (which is true) and thus it should be PCI_COMMAND_MEMORY. I think the default value should be zero and this should be overridden later in apb_pci.c.