Re: CVS commit: src/sys/dev/sdmmc
> it seems odd to have bits set in SDHC_EINTR_STATUS without > SDHC_ERROR_INTERRUPT set in SDHC_NINTR_STATUS, and > the two code paths seem different still This is because the ESDHC doesn't set SDHC_ERROR_INTERRUPT in its register since you've read SDHC_EINTR_STATUS and can see those bits directly. So, for the sdhc driver, it needs to be emulated. Don't we need to look at the error bits and take that into account before the continue? As I read the code below, if SDHC_NINTR_STATUS doesn't have SDHC_ERROR_INTERRUPT set, but some bits are set in SDHC_EINTR_STATUS, then the continue (none for us) will still be taken. Don't we still need to set the bit in status before that test? Isn't that what the code did before? If I understand, the bug was not putting SDHC_ERROR_INTERRUPT into xstatus for write-back/ack, and how the synthetic SDHC_ERROR_INTERRUPT was generated for local consumption was ok? (Sorry if I'm being dense - I'm finding this all a little hard to follow.) uint32_t xstatus = HREAD4(hp, SDHC_NINTR_STATUS); status = xstatus; error = xstatus >> 16; if (!ISSET(status, SDHC_NINTR_STATUS_MASK)) continue; /* no interrupt for us */ /* Acknowledge the interrupts we are about to handle. */ xstatus |= (error ? SDHC_ERROR_INTERRUPT : 0); HWRITE4(hp, SDHC_NINTR_STATUS, xstatus); pgp6bMiZ67Qzv.pgp Description: PGP signature
Re: CVS commit: src/sys/dev/sdmmc
On Jul 17, 2012, at 7:08 AM, Greg Troxel wrote: > So the 4-byte read causes the SDHC_ERROR_INTERRUPT not to get set because > the same read reads EINTR_STATUS, or the chip that's in systems that need > 4-byte reads is different (ESDHC vs SDHC?)? The chip does not implement SDHC_ERROR_INTERRUPT. It's always 0. It depends on the vendor. Freescale decided to not set it.
Re: CVS commit: src/sys/dev/sdmmc
On Jul 17, 2012, at 0925 , Matt Thomas wrote: > > On Jul 17, 2012, at 5:39 AM, Greg Troxel wrote: > >> > > 0x30 is aligned and that is the address that is read from a 32-bit access. Not enough coffee - I was reading hex as decimal :-) >> In the non-32 case, it seems that the EINTR register is read and then >> written back exactly if the error bit is set in the NINTR register. In >> the 32 case, it seems that the SDHC_ERROR_INTERRUPT bit is set in NINTR >> if any bits are set in EINTR, in addition to writing both. >> >> So while I see the point that the |= into status is effectively a dead >> store, >> >> it seems odd to have bits set in SDHC_EINTR_STATUS without >> SDHC_ERROR_INTERRUPT set in SDHC_NINTR_STATUS, and >> the two code paths seem different still > > This is because the ESDHC doesn't set SDHC_ERROR_INTERRUPT in its > register since you've read SDHC_EINTR_STATUS and can see those bits > directly. So, for the sdhc driver, it needs to be emulated. So the 4-byte read causes the SDHC_ERROR_INTERRUPT not to get set because the same read reads EINTR_STATUS, or the chip that's in systems that need 4-byte reads is different (ESDHC vs SDHC?)? >> I'm curious if you've been seeing bits in EINTR without ERROR in NINTR, >> and what the symptoms are that provoked this fix - I have a system with >> an sdhc (on evbppc/P2020) that mostly works but has occasional write >> errors. > > That is exactly what the ESDHC on the P2020 does. Thanks; that could be our issue then.
Re: CVS commit: src/sys/dev/sdmmc
On Jul 17, 2012, at 5:39 AM, Greg Troxel wrote: > > Modified Files: > src/sys/dev/sdmmc: sdhc.c > > Log Message: > Handle interrupt acknowledgement in the SDHC_FLAG_32BIT_ACCESS case in > the same way as non-SDHC_FLAG_32BIT_ACCESS case. > > To generate a diff of this commit: > cvs rdiff -u -r1.20 -r1.21 src/sys/dev/sdmmc/sdhc.c > > Two questions, one of which isn't about your change. > > It seems that the HREAD4 of SDHC_NINTR_STATUS is reading both that and > SDHC_EINTR_STATUS, but it seems odd that those are addresses 0x30 and > 0x32, which don't seem 4-byte aligned. So is it really ok to do an > HREAD4 across boundaries like that? It seems odd, but maybe it's about > the bus and the chip is fine with it. 0x30 is aligned and that is the address that is read from a 32-bit acccess. > In the non-32 case, it seems that the EINTR register is read and then > written back exactly if the error bit is set in the NINTR register. In > the 32 case, it seems that the SDHC_ERROR_INTERRUPT bit is set in NINTR > if any bits are set in EINTR, in addition to writing both. > > So while I see the point that the |= into status is effectively a dead > store, > > it seems odd to have bits set in SDHC_EINTR_STATUS without > SDHC_ERROR_INTERRUPT set in SDHC_NINTR_STATUS, and > the two code paths seem different still This is because the ESDHC doesn't set SDHC_ERROR_INTERRUPT in its register since you've read SDHC_EINTR_STATUS and can see those bits directly. So, for the sdhc driver, it needs to be emulated. > I'm curious if you've been seeing bits in EINTR without ERROR in NINTR, > and what the symptoms are that provoked this fix - I have a system with > an sdhc (on evbppc/P2020) that mostly works but has occasional write > errors. That is exactly what the ESDHC on the P2020 does.
Re: CVS commit: src/sys/dev/sdmmc
Modified Files: src/sys/dev/sdmmc: sdhc.c Log Message: Handle interrupt acknowledgement in the SDHC_FLAG_32BIT_ACCESS case in the same way as non-SDHC_FLAG_32BIT_ACCESS case. To generate a diff of this commit: cvs rdiff -u -r1.20 -r1.21 src/sys/dev/sdmmc/sdhc.c Two questions, one of which isn't about your change. It seems that the HREAD4 of SDHC_NINTR_STATUS is reading both that and SDHC_EINTR_STATUS, but it seems odd that those are addresses 0x30 and 0x32, which don't seem 4-byte aligned. So is it really ok to do an HREAD4 across boundaries like that? It seems odd, but maybe it's about the bus and the chip is fine with it. In the non-32 case, it seems that the EINTR register is read and then written back exactly if the error bit is set in the NINTR register. In the 32 case, it seems that the SDHC_ERROR_INTERRUPT bit is set in NINTR if any bits are set in EINTR, in addition to writing both. So while I see the point that the |= into status is effectively a dead store, it seems odd to have bits set in SDHC_EINTR_STATUS without SDHC_ERROR_INTERRUPT set in SDHC_NINTR_STATUS, and the two code paths seem different still I'm curious if you've been seeing bits in EINTR without ERROR in NINTR, and what the symptoms are that provoked this fix - I have a system with an sdhc (on evbppc/P2020) that mostly works but has occasional write errors. Thanks, Greg pgpnAXFg7lo2L.pgp Description: PGP signature
Re: CVS commit: src/sys/dev/usb
On 07/17/12 11:25, Christoph Egger wrote: > On 07/17/12 02:56, matthew green wrote: > Module Name: src Committed By: mrg Date: Sun Jul 15 21:13:31 UTC 2012 Modified Files: src/sys/dev/usb: usb_subr.c usbdi.c usbdi.h usbdivar.h Log Message: commit my workaround for PR 46648 for now, as the more involved fix is not ready yet: move the clear endpoint stall async call into the task thread, to avoid trying to call kmem_alloc() from a softint thread. XXX ideally moving callbacks into the task thread (or perhaps a different high priority task thread) would be better than this workaround, once that method is working. >>> >>> >>> I get this at boot now: >>> >>> panic: lockdebug_lookup: uninitialized lock (lock=0x80d3fe28, >>> from=806fd980) >>> fatal breakpoint trap in supervisor mode >>> trap type 1 code 0 rip 80212035 cs e030 rflags 246 cr2 0 ilevel >>> 8 rsp a00019f0d910 >>> curlwp 0xa13e2c00 pid 0 lid 26 lowest kstack 0xa00019f0a000 >>> Stopped in pid 0.26 (system) at netbsd:breakpoint+0x5: leave >>> breakpoint() at netbsd:breakpoint+0x5 >>> vpanic() at netbsd:vpanic+0x1f2 >>> printf_nolog() at netbsd:printf_nolog >>> lockdebug_wantlock() at netbsd:lockdebug_wantlock+0x18f >>> mutex_enter() at netbsd:mutex_enter+0x27b >>> usb_rem_task() at netbsd:usb_rem_task+0x2b >>> usbd_kill_pipe() at netbsd:usbd_kill_pipe+0x56 >>> usbd_new_device() at netbsd:usbd_new_device+0x41b >>> usb_doattach() at netbsd:usb_doattach+0xea >>> config_interrupts_thread() at netbsd:config_interrupts_thread+0x30 >> Fixed in usb.c rev 1.131. Christoph
Re: CVS commit: src/sys/dev/usb
On 07/17/12 02:56, matthew green wrote: >>> Module Name:src >>> Committed By: mrg >>> Date: Sun Jul 15 21:13:31 UTC 2012 >>> >>> Modified Files: >>> src/sys/dev/usb: usb_subr.c usbdi.c usbdi.h usbdivar.h >>> >>> Log Message: >>> commit my workaround for PR 46648 for now, as the more involved >>> fix is not ready yet: >>> >>> move the clear endpoint stall async call into the task thread, >>> to avoid trying to call kmem_alloc() from a softint thread. >>> >>> XXX ideally moving callbacks into the task thread (or perhaps >>> a different high priority task thread) would be better than this >>> workaround, once that method is working. >> >> >> I get this at boot now: >> >> panic: lockdebug_lookup: uninitialized lock (lock=0x80d3fe28, >> from=806fd980) >> fatal breakpoint trap in supervisor mode >> trap type 1 code 0 rip 80212035 cs e030 rflags 246 cr2 0 ilevel >> 8 rsp a00019f0d910 >> curlwp 0xa13e2c00 pid 0 lid 26 lowest kstack 0xa00019f0a000 >> Stopped in pid 0.26 (system) at netbsd:breakpoint+0x5: leave >> breakpoint() at netbsd:breakpoint+0x5 >> vpanic() at netbsd:vpanic+0x1f2 >> printf_nolog() at netbsd:printf_nolog >> lockdebug_wantlock() at netbsd:lockdebug_wantlock+0x18f >> mutex_enter() at netbsd:mutex_enter+0x27b >> usb_rem_task() at netbsd:usb_rem_task+0x2b >> usbd_kill_pipe() at netbsd:usbd_kill_pipe+0x56 >> usbd_new_device() at netbsd:usbd_new_device+0x41b >> usb_doattach() at netbsd:usb_doattach+0xea >> config_interrupts_thread() at netbsd:config_interrupts_thread+0x30 > > can you show the prior messages please? what usb devices do you have? [...] ohci0 at pci0 dev 3 function 0: ServerWorks HT1000 USB (rev. 0x01) csr: 02b00017 ohci0: interrupting at ioapic0 pin 10, event channel 10 ohci0: OHCI version 1.0, legacy support usb0 at ohci0: USB revision 1.0 usb0: WARNING: power management not supported ohci1 at pci0 dev 3 function 1: ServerWorks HT1000 USB (rev. 0x01) csr: 02b00017 ohci1: interrupting at ioapic0 pin 10, event channel 10 ohci1: OHCI version 1.0, legacy support usb1 at ohci1: USB revision 1.0 usb1: WARNING: power management not supported ehci0 at pci0 dev 3 function 2: ServerWorks HT1000 USB (rev. 0x01) ehci0: interrupting at ioapic0 pin 10, event channel 10 ehci0: companion controllers, 2 ports each: ohci0 ohci1 usb2 at ehci0: USB revision 2.0 usb2: WARNING: power management not supported [...] isa0 at pcib0 com1 at isa0 port 0x2f8-0x2ff irq 3: ns16550a, working fifo Initializing SSP: bd0cbf008749bd1a 40f970fb37887ac0 90dc8e711387bd49 21a5a4e2c1c59735 1d0d45e934bd8cce ecdb4bec302d0b3 8b7214a3b549393f dbea3b25db036a83 scsibus0: waiting 2 seconds for devices to settle... svwsata0 port 0: device present, speed: 1.5Gb/s panic: lockdebug_lookup: uninitialized lock (lock=0x80d3fe28, from=806fd910) fatal breakpoint trap in supervisor mode trap type 1 code 0 rip 80212035 cs e030 rflags 246 cr2 0 ilevel 8 rsp a00019f21930 curlwp 0xa13a6800 pid 0 lid 31 lowest kstack 0xa00019f1e000 Stopped in pid 0.31 (system) at netbsd:breakpoint+0x5: leave breakpoint() at netbsd:breakpoint+0x5 vpanic() at netbsd:vpanic+0x1f2 printf_nolog() at netbsd:printf_nolog lockdebug_wantlock() at netbsd:lockdebug_wantlock+0x18f mutex_enter() at netbsd:mutex_enter+0x27b usb_rem_task() at netbsd:usb_rem_task+0x2b usbd_kill_pipe() at netbsd:usbd_kill_pipe+0x56 usbd_new_device() at netbsd:usbd_new_device+0x41b usb_doattach() at netbsd:usb_doattach+0xe1 config_interrupts_thread() at netbsd:config_interrupts_thread+0x30 Christoph
