Re: OHCI IRQ issues

On Sat, 24 Nov 2007, Benjamin Herrenschmidt wrote:

> I have a problem with the current ohci_irq() implementation:
> 
> - The optimisation below breaks if the controller has an edge triggered
> interrupt which is the case on some embedded controllers such as the
> AMCC PowerPC 440EP:
> 
> 	/* we can eliminate a (slow) ohci_readl()
> 	 * if _only_ WDH caused this irq
> 	 */
> 	if ((ohci->hcca->done_head != 0)
> 			&& ! (hc32_to_cpup (ohci, &ohci->hcca->done_head)
> 				& 0x01)) {
> 		ints =  OHCI_INTR_WDH;
> 
> 	/* cardbus/... hardware gone before remove() */
> 	} else if ((ints = ohci_readl (ohci, &regs->intrstatus)) == ~(u32)0) {
> 		disable (ohci);
> 		ohci_dbg (ohci, "device removed!\n");
> 		return IRQ_HANDLED;
> 
> We must not do that on edge interrupts because the interrupts we "miss"
> will not be re-triggered as they don't stay asserted.

I'm confused.  Are you saying that the USB controller generates a
normal interrupt request level but the interrupt controller interprets 
it as edge-triggered?  How could that ever possibly work?

Or are you saying that the USB controller generates a "spike" interrupt 
request signal whenever one of the appropriate bits gets turned on?  In 
that case the interrupts we miss _would_ be re-triggered, wouldn't 
they?

> - There may also be issues with edge interrupts here:
> 
> 	if (ints & OHCI_INTR_RHSC) {
> 
> 	.../...
> 
> 	}
> 	else if (ints & OHCI_INTR_RD) {
> 
> If for some reason both RHSC and RD are set, we'll never notice RD, is
> that expected ?

Yes, it is.  The RHSC case calls usb_hcd_poll_rh_status(), which 
indirectly calls ohci_root_hub_state_changes(), which does a resume if 
the controller is suspended or auto-stopped.  Hence in this case the RD 
isn't needed or wanted.

> - I can't understand why we do that masking/unmasking later on:
> 
> 	if (ints & OHCI_INTR_WDH) {
> 		if (HC_IS_RUNNING(hcd->state))
> 			ohci_writel (ohci, OHCI_INTR_WDH, &regs->intrdisable);
> 		spin_lock (&ohci->lock);
> 		dl_done_list (ohci);
> 		spin_unlock (&ohci->lock);
> 		if (HC_IS_RUNNING(hcd->state))
> 			ohci_writel (ohci, OHCI_INTR_WDH, &regs->intrenable);
> 	}
> 
> Those 2 writes to intrdisable look totally useless to me. First they are
> posted so they won't actually protect anything, then, we -are- in the
> interrupt handler for the OHCI, so we cannot take that interrupt again
> recursively anyway. So they are just a waste of cycles as far as I'm
> concerned.

Me too.  I don't know why those lines are there.

> Now, regarding the first problem, my proposal would be to just remove
> the optimisation, I can't see it being very useful in practice. We
> -could- add some flag whether the driver is using a level or an edge
> interrupt and have the SoC glue set it, but that doesn't seem like a
> worthy optimisation to me. Beside, that's also going to get us into
> trouble if the implementation has subtle DMA ordering issues, i'd rather
> -guarantee- that DMAs are flushed with a read before going to mess
> around with the done list (I suspect that may explain some so far
> unexplained problem we've been having on some other weird
> implementations).

If it does cause problems on some chipsets then removing it is 
probably okay.  It looks like a very minor optimization.

> As for the two others, what do you guys think ? I believe the masking in
> WDH handling can just be removed, and I'm not sure about the else around
> RD handling.

The "else" around RD is really needed, or at least it used to be
needed.  Without it we would end up trying to resume a suspended
controller twice, and the second resume would get into trouble.

Alan Stern


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel