Patch review [was: Re: viapropm doesnt like sys/dev/pci.c rev 1.214]
On Thu, Jun 05, 2003 at 04:17:38AM -0700, Terry Lambert wrote:
How about RF_DONTCHECK or RF_ALWAYSWORKS? It better implies
what's happening here, since you're going to assume success in
the face of diagnostics to the contrary.
So instead of:
if (flag)
return (0);
command = PCI_READ_CONFIG(dev, child, PCIR_COMMAND, 2);
if (command bit)
return (0);
device_printf(child, failed to enable %s mapping!\n, error);
return (ENXIO);
You do:
command = PCI_READ_CONFIG(dev, child, PCIR_COMMAND, 2);
if ((command bit) || flag)
return (0);
device_printf(child, failed to enable %s mapping!\n, error);
return (ENXIO);
Yeah, I know the disctinction is subtle, but there migh be other
PCI_READ_CONFIG() results later that people care about, besides
just this one bit, which *do* work on some other chip with the
same issue.
Sounds good like that? (ignore more changes in amdpm.c. Just consider
that RF_DONTCHECK was added to the resource allocation). Note that
AMD-768 PM has the same flaw as the VIA chipset.
Index: dev/cardbus/cardbus_cis.c
===
RCS file: /home/ncvs/src/sys/dev/cardbus/cardbus_cis.c,v
retrieving revision 1.37
diff -u -r1.37 cardbus_cis.c
--- dev/cardbus/cardbus_cis.c 24 May 2003 23:23:41 - 1.37
+++ dev/cardbus/cardbus_cis.c 15 Jun 2003 16:05:16 -
@@ -457,7 +457,7 @@
* Mark the appropriate bit in the PCI command register so that
* device drivers will know which type of BARs can be used.
*/
- pci_enable_io(child, type);
+ pci_enable_io(child, type, 0);
return (0);
}
@@ -624,7 +624,7 @@
rman_get_start(res) | ((*rid == CARDBUS_ROM_REG)?
CARDBUS_ROM_ENABLE : 0),
4);
- PCI_ENABLE_IO(cbdev, child, SYS_RES_MEMORY);
+ PCI_ENABLE_IO(cbdev, child, SYS_RES_MEMORY, 0);
/* Flip to the right ROM image if CIS is in ROM */
if (CARDBUS_CIS_SPACE(*start) == CARDBUS_CIS_ASI_ROM) {
Index: dev/hifn/hifn7751.c
===
RCS file: /home/ncvs/src/sys/dev/hifn/hifn7751.c,v
retrieving revision 1.13
diff -u -r1.13 hifn7751.c
--- dev/hifn/hifn7751.c 11 Mar 2003 22:47:06 - 1.13
+++ dev/hifn/hifn7751.c 15 Jun 2003 16:03:43 -
@@ -616,7 +616,7 @@
/* reenable busmastering */
pci_enable_busmaster(dev);
- pci_enable_io(dev, HIFN_RES);
+ pci_enable_io(dev, HIFN_RES, 0);
/* reinitialize interface if necessary */
if (ifp-if_flags IFF_UP)
Index: dev/pci/pci.c
===
RCS file: /home/ncvs/src/sys/dev/pci/pci.c,v
retrieving revision 1.214
diff -u -r1.214 pci.c
--- dev/pci/pci.c 16 Apr 2003 03:15:08 - 1.214
+++ dev/pci/pci.c 15 Jun 2003 15:25:57 -
@@ -583,7 +583,7 @@
}
int
-pci_enable_io_method(device_t dev, device_t child, int space)
+pci_enable_io_method(device_t dev, device_t child, int space, u_int flags)
{
u_int16_t command;
u_int16_t bit;
@@ -607,7 +607,7 @@
}
pci_set_command_bit(dev, child, bit);
command = PCI_READ_CONFIG(dev, child, PCIR_COMMAND, 2);
- if (command bit)
+ if ((command bit) || (flags RF_DONTCHECK))
return (0);
device_printf(child, failed to enable %s mapping!\n, error);
return (ENXIO);
@@ -1365,7 +1365,7 @@
* Enable the I/O mode. We should also be allocating
* resources too. XXX
*/
- if (PCI_ENABLE_IO(dev, child, type))
+ if (PCI_ENABLE_IO(dev, child, type, flags))
return (NULL);
break;
}
Index: dev/pci/pci_if.m
===
RCS file: /home/ncvs/src/sys/dev/pci/pci_if.m,v
retrieving revision 1.5
diff -u -r1.5 pci_if.m
--- dev/pci/pci_if.m16 Apr 2003 03:15:08 - 1.5
+++ dev/pci/pci_if.m15 Jun 2003 15:23:23 -
@@ -70,6 +70,7 @@
device_tdev;
device_tchild;
int space;
+ u_int flags;
};
METHOD int disable_io {
Index: dev/pci/pci_private.h
===
RCS file: /home/ncvs/src/sys/dev/pci/pci_private.h,v
retrieving revision 1.8
diff -u -r1.8 pci_private.h
--- dev/pci/pci_private.h 16 Apr 2003 03:15:08 - 1.8
+++ dev/pci/pci_private.h 15 Jun 2003 15:27:55 -
@@ -56,7 +56,8 @@
int reg, u_int32_t val, int width);
intpci_enable_busmaster_method(device_t dev, device_t child);
intpci_disable_busmaster_method(device_t dev, device_t child);
-int
Re: viapropm doesnt like sys/dev/pci.c rev 1.214
David P. Reese Jr. wrote: A snip of code from sys/dev/pci/pci.c:pci_enable_io_method(): pci_set_command_bit(dev, child, bit); command = PCI_READ_CONFIG(dev, child, PCIR_COMMAND, 2); if (command bit) return (0); device_printf(child, failed to enable %s mapping!\n, error); return (ENXIO); Because the viapropm's command register bits will always read as zero, this code will always fail when trying to enable port mapping. [ ... ] How about adding another flag to bus_alloc_resource() which would signal that we are not to check the value of the command register after calling pci_set_command_bit(). RF_WILLFAIL? After pci_enable_io_method() gets swallowed into pci_alloc_resource(), this would be pretty easy because the flag would be in scope when we check the value of the command register. I can do so this weekend if anyone thinks this is worthwhile. How about RF_DONTCHECK or RF_ALWAYSWORKS? It better implies what's happening here, since you're going to assume success in the face of diagnostics to the contrary. So instead of: if (flag) return (0); command = PCI_READ_CONFIG(dev, child, PCIR_COMMAND, 2); if (command bit) return (0); device_printf(child, failed to enable %s mapping!\n, error); return (ENXIO); You do: command = PCI_READ_CONFIG(dev, child, PCIR_COMMAND, 2); if ((command bit) || flag) return (0); device_printf(child, failed to enable %s mapping!\n, error); return (ENXIO); Yeah, I know the disctinction is subtle, but there migh be other PCI_READ_CONFIG() results later that people care about, besides just this one bit, which *do* work on some other chip with the same issue. -- Terry ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: viapropm doesnt like sys/dev/pci.c rev 1.214
On Tue, Jun 03, 2003 at 01:54:36AM -0500, Conrad Sabatier wrote: On 02-Jun-2003 Nicolas Souchu wrote: On Sun, Jun 01, 2003 at 01:52:57AM +0200, Dag-Erling Smorgrav wrote: viapropm is seriously broken for other reasons and needs professional help. What kind of breakage? Setting resources in probe? Right. Anybody having the viapm driver loaded usually should please try the attached patch. I'm sorry to report that those patches didn't fix the problem for me. They all applied cleanly, I built a new kernel, but I still see the same messages at boot. Couldn't enable port mapping. The problem is a disagreement with the new io_method code and the viapropm chip. From Nicolas Souchu's previous email: : The datasheet states that the command bits are RW but fixed at 0. A snip of code from sys/dev/pci/pci.c:pci_enable_io_method(): pci_set_command_bit(dev, child, bit); command = PCI_READ_CONFIG(dev, child, PCIR_COMMAND, 2); if (command bit) return (0); device_printf(child, failed to enable %s mapping!\n, error); return (ENXIO); Because the viapropm's command register bits will always read as zero, this code will always fail when trying to enable port mapping. Whatever problems viapropm may have, it is the new pci code that prevents it from attaching. It is not the fault of anything in sys/pci/viapm.c. -- David P. Reese Jr. [EMAIL PROTECTED] -- It can be argued that returning a NULL pointer when asked to allocate zero bytes is a silly response to a silly question. -- FreeBSD manual page for malloc(3) ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: viapropm doesnt like sys/dev/pci.c rev 1.214
On Tue, Jun 03, 2003 at 10:54:30AM -0700, David P. Reese Jr. wrote: [...] : The datasheet states that the command bits are RW but fixed at 0. A snip of code from sys/dev/pci/pci.c:pci_enable_io_method(): pci_set_command_bit(dev, child, bit); command = PCI_READ_CONFIG(dev, child, PCIR_COMMAND, 2); if (command bit) return (0); device_printf(child, failed to enable %s mapping!\n, error); return (ENXIO); Because the viapropm's command register bits will always read as zero, this code will always fail when trying to enable port mapping. Whatever problems viapropm may have, it is the new pci code that prevents it from attaching. It is not the fault of anything in sys/pci/viapm.c. And I personally don't know how to fix it except by an option with an ifdef to workaround it. -- Nicholas Souchu - [EMAIL PROTECTED] - [EMAIL PROTECTED] ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: viapropm doesnt like sys/dev/pci.c rev 1.214
On Wed, Jun 04, 2003 at 07:29:31AM +, Nicolas Souchu wrote: On Tue, Jun 03, 2003 at 10:54:30AM -0700, David P. Reese Jr. wrote: [...] : The datasheet states that the command bits are RW but fixed at 0. A snip of code from sys/dev/pci/pci.c:pci_enable_io_method(): pci_set_command_bit(dev, child, bit); command = PCI_READ_CONFIG(dev, child, PCIR_COMMAND, 2); if (command bit) return (0); device_printf(child, failed to enable %s mapping!\n, error); return (ENXIO); Because the viapropm's command register bits will always read as zero, this code will always fail when trying to enable port mapping. Whatever problems viapropm may have, it is the new pci code that prevents it from attaching. It is not the fault of anything in sys/pci/viapm.c. And I personally don't know how to fix it except by an option with an ifdef to workaround it. How about adding another flag to bus_alloc_resource() which would signal that we are not to check the value of the command register after calling pci_set_command_bit(). RF_WILLFAIL? After pci_enable_io_method() gets swallowed into pci_alloc_resource(), this would be pretty easy because the flag would be in scope when we check the value of the command register. I can do so this weekend if anyone thinks this is worthwhile. -- David P. Reese Jr. [EMAIL PROTECTED] -- It can be argued that returning a NULL pointer when asked to allocate zero bytes is a silly response to a silly question. -- FreeBSD manual page for malloc(3) ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: viapropm doesnt like sys/dev/pci.c rev 1.214
On Sun, Jun 01, 2003 at 01:52:57AM +0200, Dag-Erling Smorgrav wrote:
David P. Reese Jr. [EMAIL PROTECTED] writes:
In rev 1.214 of sys/dev/pci/pci.c, we have started checking if a
pci_set_command_bit() was successful with a subsequent PCI_READ_CONFIG
and comparing the results. For some odd reason, this doesnt work when
my viapropm tries to attach.
viapropm is seriously broken for other reasons and needs professional
help.
What kind of breakage? Setting resources in probe? Right. Anybody having
the viapm driver loaded usually should please try the attached patch.
pci_set_command_bit(dev, child, bit);
command = PCI_READ_CONFIG(dev, child, PCIR_COMMAND, 2);
if (command bit)
return (0);
It should allow the register to settle between write and read, which
may take some time (see chipset docs for timing details). DELAY(1000)
should be OK in an attach function.
The datasheet states that the command bits are RW but fixed at 0.
Nicholas
--
Nicholas Souchu - [EMAIL PROTECTED] - [EMAIL PROTECTED]
Index: viapm.c
===
RCS file: /home/ncvs/src/sys/pci/viapm.c,v
retrieving revision 1.2
diff -u -r1.2 viapm.c
--- viapm.c 9 Nov 2002 20:13:16 - 1.2
+++ viapm.c 25 May 2003 22:00:03 -
@@ -79,7 +79,6 @@
#define VIAPM_TYP_8233 5
struct viapm_softc {
- int type;
u_int32_t base;
bus_space_tag_t st;
bus_space_handle_t sh;
@@ -179,137 +178,42 @@
static int
viapm_586b_probe(device_t dev)
{
- struct viapm_softc *viapm = (struct viapm_softc *)device_get_softc(dev);
- u_int32_t l;
- u_int16_t s;
- u_int8_t c;
+ if (pci_get_devid(dev) != VIA_586B_PMU_ID)
+ return ENXIO;
- switch (pci_get_devid(dev)) {
- case VIA_586B_PMU_ID:
-
- bzero(viapm, sizeof(struct viapm_softc));
-
- l = pci_read_config(dev, VIAPM_586B_REVID, 1);
- switch (l) {
- case VIAPM_586B_OEM_REV_E:
- viapm-type = VIAPM_TYP_586B_3040E;
- viapm-iorid = VIAPM_586B_3040E_BASE;
-
- /* Activate IO block access */
- s = pci_read_config(dev, VIAPM_586B_3040E_ACTIV, 2);
- pci_write_config(dev, VIAPM_586B_3040E_ACTIV, s | 0x1, 2);
- break;
-
- case VIAPM_586B_OEM_REV_F:
- case VIAPM_586B_PROD_REV_A:
- default:
- viapm-type = VIAPM_TYP_586B_3040F;
- viapm-iorid = VIAPM_586B_3040F_BASE;
-
- /* Activate IO block access */
- c = pci_read_config(dev, VIAPM_586B_3040F_ACTIV, 1);
- pci_write_config(dev, VIAPM_586B_3040F_ACTIV, c | 0x80, 1);
- break;
- }
-
- viapm-base = pci_read_config(dev, viapm-iorid, 4)
- VIAPM_586B_BA_MASK;
-
- /*
-* We have to set the I/O resources by hand because it is
-* described outside the viapmope of the traditional maps
-*/
- if (bus_set_resource(dev, SYS_RES_IOPORT, viapm-iorid,
- viapm-base, 256)) {
- device_printf(dev, could not set bus resource\n);
- return ENXIO;
- }
- device_set_desc(dev, VIA VT82C586B Power Management Unit);
- return 0;
-
- default:
- break;
- }
-
- return ENXIO;
+ device_set_desc(dev, VIA VT82C586B Power Management Unit);
+ return 0;
}
-
static int
viapm_pro_probe(device_t dev)
{
- struct viapm_softc *viapm = (struct viapm_softc *)device_get_softc(dev);
-#ifdef VIAPM_BASE_ADDR
- u_int32_t l;
-#endif
- u_int32_t base_cfgreg;
char *desc;
switch (pci_get_devid(dev)) {
case VIA_596A_PMU_ID:
desc = VIA VT82C596A Power Management Unit;
- viapm-type = VIAPM_TYP_596B;
- base_cfgreg = VIAPM_PRO_BASE;
- goto viapro;
+ break;
case VIA_596B_PMU_ID:
desc = VIA VT82C596B Power Management Unit;
- viapm-type = VIAPM_TYP_596B;
- base_cfgreg = VIAPM_PRO_BASE;
- goto viapro;
+ break;
case VIA_686A_PMU_ID:
desc = VIA VT82C686A Power Management Unit;
- viapm-type = VIAPM_TYP_686A;
- base_cfgreg = VIAPM_PRO_BASE;
- goto viapro;
+ break;
case VIA_8233_PMU_ID:
desc = VIA VT8233 Power Management Unit;
- viapm-type = VIAPM_TYP_UNKNOWN;
- base_cfgreg = VIAPM_8233_BASE;
- goto viapro;
-
-
Re: viapropm doesnt like sys/dev/pci.c rev 1.214
On 02-Jun-2003 Nicolas Souchu wrote: On Sun, Jun 01, 2003 at 01:52:57AM +0200, Dag-Erling Smorgrav wrote: viapropm is seriously broken for other reasons and needs professional help. What kind of breakage? Setting resources in probe? Right. Anybody having the viapm driver loaded usually should please try the attached patch. I'm sorry to report that those patches didn't fix the problem for me. They all applied cleanly, I built a new kernel, but I still see the same messages at boot. Couldn't enable port mapping. Thanks anyway, though. :-) -- Conrad Sabatier [EMAIL PROTECTED] - In Unix veritas pgp0.pgp Description: PGP signature
viapropm doesnt like sys/dev/pci.c rev 1.214
In rev 1.214 of sys/dev/pci/pci.c, we have started checking if a pci_set_command_bit() was successful with a subsequent PCI_READ_CONFIG and comparing the results. For some odd reason, this doesnt work when my viapropm tries to attach. Allocating its port resources fails in pci_enable_io_method(). The code in question is: sys/dev/pci/pci.c:pci_enable_io_method() [snip] pci_set_command_bit(dev, child, bit); command = PCI_READ_CONFIG(dev, child, PCIR_COMMAND, 2); if (command bit) return (0); device_printf(child, failed to enable %s mapping!\n, error); return (ENXIO); What is annoying is that by changing the last line to return a zero instead of ENXIO, the viapropm successfully attaches. The smbus interface even works. Everything else that tries to claim port resources succeeds. Is it my chipset's fault for not reading back the correct register value? The board is a SOYO K7VTAPRO-2AA6. What other info would be helpful in this situation? [EMAIL PROTECTED]:7:4: class=0x068000 card=0x30571106 chip=0x30571106 rev=0x40 hdr=0x00 vendor = 'VIA Technologies Inc' device = 'VT82C686A/B ACPI Power Management Controller' class= bridge subclass = PCI-unknown -- David P. Reese Jr. [EMAIL PROTECTED] -- It can be argued that returning a NULL pointer when asked to allocate zero bytes is a silly response to a silly question. -- FreeBSD manual page for malloc(3) ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: viapropm doesnt like sys/dev/pci.c rev 1.214
In message: [EMAIL PROTECTED] David P. Reese Jr. [EMAIL PROTECTED] writes: : Is it my chipset's fault for not reading back the correct register value? : The board is a SOYO K7VTAPRO-2AA6. What other info would be helpful in : this situation? It appears that there's some non-zero latency between when we command the bits to change and they really change. A number of people have reported this, but I'm not what the right way to fix it is. Warner ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: viapropm doesnt like sys/dev/pci.c rev 1.214
David P. Reese Jr. [EMAIL PROTECTED] writes: In rev 1.214 of sys/dev/pci/pci.c, we have started checking if a pci_set_command_bit() was successful with a subsequent PCI_READ_CONFIG and comparing the results. For some odd reason, this doesnt work when my viapropm tries to attach. viapropm is seriously broken for other reasons and needs professional help. pci_set_command_bit(dev, child, bit); command = PCI_READ_CONFIG(dev, child, PCIR_COMMAND, 2); if (command bit) return (0); It should allow the register to settle between write and read, which may take some time (see chipset docs for timing details). DELAY(1000) should be OK in an attach function. DES -- Dag-Erling Smorgrav - [EMAIL PROTECTED] ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: viapropm doesnt like sys/dev/pci.c rev 1.214
On Sun, Jun 01, 2003 at 01:52:57AM +0200, Dag-Erling Smorgrav wrote: David P. Reese Jr. [EMAIL PROTECTED] writes: In rev 1.214 of sys/dev/pci/pci.c, we have started checking if a pci_set_command_bit() was successful with a subsequent PCI_READ_CONFIG and comparing the results. For some odd reason, this doesnt work when my viapropm tries to attach. viapropm is seriously broken for other reasons and needs professional help. It will be hard for me to provide that professional help because the chipset docs require an NDA. pci_set_command_bit(dev, child, bit); command = PCI_READ_CONFIG(dev, child, PCIR_COMMAND, 2); if (command bit) return (0); It should allow the register to settle between write and read, which may take some time (see chipset docs for timing details). DELAY(1000) should be OK in an attach function. Well, using the following patch: Index: pci.c === RCS file: /home/daver/cvs-freebsd/src/sys/dev/pci/pci.c,v retrieving revision 1.214 diff -u -r1.214 pci.c --- pci.c 16 Apr 2003 03:15:08 - 1.214 +++ pci.c 1 Jun 2003 02:45:11 - @@ -606,6 +606,9 @@ break; } pci_set_command_bit(dev, child, bit); +#define PCI_CFG_DELAY 1000 + device_printf(dev, delaying for %i microseconds\n, PCI_CFG_DELAY); + DELAY(PCI_CFG_DELAY); command = PCI_READ_CONFIG(dev, child, PCIR_COMMAND, 2); if (command bit) return (0); I get: viapropm0: SMBus I/O base at 0x5000 viapropm0: VIA VT82C686A Power Management Unit port 0x5000-0x500f at device 7.4 on pci0 pci0: delaying for 1000 microseconds viapropm0: failed to enable port mapping! viapropm0: could not allocate bus space device_probe_and_attach: viapropm0 attach returned 6 This seems to imply that we don't have a timing problem. Instead it looks like the chip doesn't want reflect it's status in it's command register. Can someone with access to the docs give me a clue? -- David P. Reese Jr. [EMAIL PROTECTED] -- It can be argued that returning a NULL pointer when asked to allocate zero bytes is a silly response to a silly question. -- FreeBSD manual page for malloc(3) ___ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to [EMAIL PROTECTED]
