From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MxkIg-0004gv-5p for qemu-devel@nongnu.org; Tue, 13 Oct 2009 12:32:38 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MxkIb-0004ay-K2 for qemu-devel@nongnu.org; Tue, 13 Oct 2009 12:32:37 -0400 Received: from [199.232.76.173] (port=57691 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MxkIb-0004aZ-5T for qemu-devel@nongnu.org; Tue, 13 Oct 2009 12:32:33 -0400 Received: from mail-fx0-f214.google.com ([209.85.220.214]:47829) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MxkIa-0006Wq-CM for qemu-devel@nongnu.org; Tue, 13 Oct 2009 12:32:32 -0400 Received: by fxm10 with SMTP id 10so9252602fxm.8 for ; Tue, 13 Oct 2009 09:32:31 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <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> <20091013152610.GA20492@redhat.com> From: Blue Swirl Date: Tue, 13 Oct 2009 19:32:11 +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: "Michael S. Tsirkin" Cc: Isaku Yamahata , qemu-devel@nongnu.org On Tue, Oct 13, 2009 at 6:26 PM, Michael S. Tsirkin wrote: > 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 >> >> > --- >> >> > =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 mas= ter, pci mem >> >> > - =C2=A0 =C2=A0s->dev.config[0x05] =3D 0x00; >> >> > - =C2=A0 =C2=A0s->dev.config[0x06] =3D 0xa0; // status =3D fast bac= k-to-back, 66MHz, no error >> >> > - =C2=A0 =C2=A0s->dev.config[0x07] =3D 0x00; // status =3D fast dev= sel >> >> > - =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= _PCI); >> >> > - =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_H= EADER_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_COMMA= ND_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 funct= ion >> > 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) > > Link? http://www.sun.com/processors/manuals/805-1251.pdf > Maybe we should put spec links in comments > in relevant code ... Yes. Maybe that should be mandatory for new code.