| Message ID | 1255069742-15724-8-git-send-email-yamahata@valinux.co.jp |
|---|---|
| State | Under Review |
| Headers | show |
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? > + 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. > + 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; > } > > -- > 1.6.0.2
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> Posted some minor comments previously, but anyway, this is much better than what we now have. Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > 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); > + 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; > + 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; > } > > -- > 1.6.0.2
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? > > + 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; > > } > > >
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
On Tue, Oct 13, 2009 at 4:20 PM, Isaku Yamahata <yamahata@valinux.co.jp> 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? 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.
On Tue, Oct 13, 2009 at 06:12:17PM +0300, Blue Swirl wrote: > On Tue, Oct 13, 2009 at 4:20 PM, Isaku Yamahata <yamahata@valinux.co.jp> 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? > > 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. > > I think the default value should be zero and this should be overridden > later in apb_pci.c. Makes sense.
On Tue, Oct 13, 2009 at 6:26 PM, Michael S. Tsirkin <mst@redhat.com> 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 <yamahata@valinux.co.jp> 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? >> >> 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.
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); + 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; + 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; }
- 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(-)