Subject: kern/27423: SERR and PARITY of PCI enable unconditionally anytime?
To: None <gnats-bugs@gnats.NetBSD.org>
From: None <kiyohara@kk.iij4u.or.jp>
List: netbsd-bugs
Date: 10/24/2004 15:27:33
>Number: 27423
>Category: kern
>Synopsis: SERR and PARITY of PCI enable unconditionally anytime?
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: kern-bug-people
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: change-request
>Submitter-Id: net
>Arrival-Date: Sun Oct 24 15:28:01 UTC 2004
>Closed-Date:
>Last-Modified:
>Originator: KIYOHARA Takashi
>Release: 2.99.10
>Organization:
>Environment:
NetBSD highpriestess.fool 2.99.10 NetBSD 2.99.10 (HIGHPRIESTESS) #0: Sun Oct 17 18:29:20 JST 2004 lance@highpriestess.fool:/sys/arch/i386/compile/HIGHPRIESTESS i386
>Description:
SERR and PARITY of PCI enable unconditionally anytime?
However, PCI_CONF_ENABLE_MEM and PCI_CONF_ENABLE_IO are enable conditionally.
I think it strange.
cobalt Qube has a problem about it.
Furthermore, there is mail with which tsutsui@netbsd.org described
the problem of PCI_COMMAND_MEM_ENABLE and PCI_COMMAND_IO_ENABLE.
-----
Subject: Re: Initialization of PCI
From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: port-cobalt@netbsd.org
Date: Wed, 25 Aug 2004 19:55:34 +0900
In article <20040825.003452.74747022.kiyohara@kk.iij4u.or.jp>
kiyohara@kk.iij4u.or.jp wrote:
> From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
> Date: Tue, 24 Aug 2004 23:23:10 +0900
>
> > In article <20040824.013249.104033134.kiyohara@kk.iij4u.or.jp>
> > kiyohara@kk.iij4u.or.jp wrote:
:
> > Ah, I see, you mean that the firmware doesn't set
> > PCI_COMMAND_SERR_ENABLE and PCI_COMMAND_PARITY_ENABLE
> > but sys/dev/pci/pciconf.c:configure_bus() sets them implicitly,
> > so we have to disable them again after pci_configure_bus(9), right?
> >
> > Hmm, I wonder if we should have some flags (in pci_conf_hook()?)
> > to disable them in MI pci_configure_bus(), but for now I think
> > it's okay to do it for each devices in gt_attach().
:
> I am making the following change.
> It will also influence MI. However, I think it strange that it is enabled
> unconditionally.
>
> MI must demand change.
Well, it's always good thing to make things done in MI,
but if we would like to change MI code, we have to consult
responsible people first ;-)
I thought the similar fix with your patch (adding PCI_CONF_ENABLE_PARITY
and PCI_CONF_ENABLE_SERR in pciconf.h), but I wonder if the changes
breaks existing code because:
- pciconf.c:setup_iowins() and setup_memwins() could implicitly
set pd->enable = 0 so added flags might be cleared:
if (!pb->io_32bit && pi->address > 0xFFFF) {
pi->address = 0;
pd->enable = 0;
}
:
if (pm->prefetch && !pb->pmem_64bit &&
pm->address > 0xFFFFFFFFULL) {
pm->address = 0;
pd->enable = 0;
- and pciconf.c:configure_bus() checks if pd->enable is zero:
if (!(pd->enable)) {
print_tag(pd->pc, pd->tag);
printf("Disabled due to lack of resources.\n");
cmd &= ~(PCI_COMMAND_MASTER_ENABLE |
PCI_COMMAND_IO_ENABLE | PCI_COMMAND_MEM_ENABLE);
}
(BTW, setup_iowins() always set PCI_CONF_ENABLE_IO even
after pd->enable is cleared...)
I guess setup_iowins() and setup_menwins() should clear only
PCI_COMMAND_MEM_ENABLE or PCI_COMMAND_IO_ENABLE and
configure_bus() should check only these two flags, but I'm not sure.
Allen, Jason, how do you think about this change?
---
Izumi Tsutsui
tsutsui@ceres.dti.ne.jp
>How-To-Repeat:
>Fix:
Index: pciconf.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/pciconf.c,v
retrieving revision 1.23
diff -u -r1.23 pciconf.c
--- pciconf.c 17 Mar 2004 20:27:57 -0000 1.23
+++ pciconf.c 25 Aug 2004 10:50:30 -0000
@@ -705,10 +705,6 @@
PRIu64 " req)\n", pi->size);
return -1;
}
- if (!pb->io_32bit && pi->address > 0xFFFF) {
- pi->address = 0;
- pd->enable = 0;
- }
if (pd->ppb && pi->reg == 0) {
pd->ppb->ioext = extent_create("pciconf", pi->address,
pi->address + pi->size, M_DEVBUF, NULL, 0,
@@ -721,7 +717,12 @@
}
continue;
}
- pd->enable |= PCI_CONF_ENABLE_IO;
+ if (!pb->io_32bit && pi->address > 0xFFFF) {
+ pi->address = 0;
+ pd->enable &= ~PCI_CONF_ENABLE_IO;
+ } else {
+ pd->enable |= PCI_CONF_ENABLE_IO;
+ }
if (pci_conf_debug) {
print_tag(pd->pc, pd->tag);
printf("Putting %" PRIu64 " I/O bytes @ %#" PRIx64
@@ -775,7 +776,7 @@
if (pm->prefetch && !pb->pmem_64bit &&
pm->address > 0xFFFFFFFFULL) {
pm->address = 0;
- pd->enable = 0;
+ pd->enable &= ~PCI_CONF_ENABLE_MEM;
} else {
pd->enable |= PCI_CONF_ENABLE_MEM;
}
@@ -1005,7 +1006,10 @@
class = pci_conf_read(pd->pc, pd->tag, PCI_CLASS_REG);
misc = pci_conf_read(pd->pc, pd->tag, PCI_BHLC_REG);
cmd = pci_conf_read(pd->pc, pd->tag, PCI_COMMAND_STATUS_REG);
- cmd |= PCI_COMMAND_SERR_ENABLE | PCI_COMMAND_PARITY_ENABLE;
+ if (pd->enable & PCI_CONF_ENABLE_PARITY)
+ cmd |= PCI_COMMAND_PARITY_ENABLE;
+ if (pd->enable & PCI_CONF_ENABLE_SERR)
+ cmd |= PCI_COMMAND_SERR_ENABLE;
if (pb->fast_b2b)
cmd |= PCI_COMMAND_BACKTOBACK_ENABLE;
if (PCI_CLASS(class) != PCI_CLASS_BRIDGE ||
@@ -1022,7 +1026,8 @@
cmd |= PCI_COMMAND_MASTER_ENABLE;
ltim = MIN (pb->def_ltim, pb->max_ltim);
}
- if (!(pd->enable)) {
+ if ((pd->enable &
+ (PCI_CONF_ENABLE_MEM|PCI_CONF_ENABLE_IO)) == 0) {
print_tag(pd->pc, pd->tag);
printf("Disabled due to lack of resources.\n");
cmd &= ~(PCI_COMMAND_MASTER_ENABLE |
Index: pciconf.h
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/pciconf.h,v
retrieving revision 1.7
diff -u -r1.7 pciconf.h
--- pciconf.h 28 Sep 2002 10:31:02 -0000 1.7
+++ pciconf.h 25 Aug 2004 10:50:30 -0000
@@ -55,5 +55,7 @@
#define PCI_CONF_ENABLE_IO 0x08
#define PCI_CONF_ENABLE_MEM 0x10
#define PCI_CONF_ENABLE_BM 0x20
+#define PCI_CONF_ENABLE_PARITY 0x40
+#define PCI_CONF_ENABLE_SERR 0x80
-#define PCI_CONF_ALL 0x3f
+#define PCI_CONF_ALL 0xff
>Release-Note:
>Audit-Trail:
>Unformatted: