usb: xhci: Simplify clearing the Event Interrupt bit

USBSTS is mostly RW1C, so to clear EINT we should write just this one bit. Remove pointless code which ORs the bit with current value of the register, even though the bit is already known to be set, and writes the result back, which clears all active RW1C flags.

We used to inadvertently clear PCD and SRE in this way. PCD isn't used by the driver and SRE is only used at resume, so clearing them should make no difference. Don't clear them anymore.

Tested by connecting and mounting a storage device on a few HCs.

Before: xhci_irq USBSTS 0x00000018 EINT PCD -> 0x00000000

        xhci_irq USBSTS 0x00000008 EINT -> 0x00000000
After:  xhci_irq USBSTS 0x00000018 EINT PCD -> 0x00000010 PCD
        xhci_irq USBSTS 0x00000018 EINT PCD -> 0x00000010 PCD

Some flags are RsvdZ - should be written as zero regardless of the value read, so technically it was a bug. But no problems are known.

---
 drivers/usb/host/xhci-ring.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index 246a2b1573ff..0338daba5f9a 100644
@@ -3209,10 +3209,9 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
 	/*
 	 * Clear the op reg interrupt status first,
 	 * so we can receive interrupts from other MSI-X interrupters.
-	 * Write 1 to clear the interrupt status.
+	 * USBSTS bits are write 1 to clear.
 	 */
-	status |= STS_EINT;
-	writel(status, &xhci->op_regs->status);
+	writel(STS_EINT, &xhci->op_regs->status);
 
 	/* This is the handler of the primary interrupter */
 	xhci_handle_events(xhci, xhci->interrupters[0], false);

2.48.1

Thanks for cleaning this up.

Leaving the Port Change Detectd (PCD) bit uncleared worries me a bit. Did suspend and resume still work properly after this? especially something like wakeup from suspend (D3) on usb port connect

Thanks
Mathias

The bit seems to only be mentioned in 5.4.2 and 4.15.2.3, with no indication that it's supposed to have any effect on HW behavior.

And I found no occurences of STS_PORT outside xhci.h.

But to be sure, I tested resuming by connection today.

Chipset xHCI and a PCIe card with Etron are fully functional.

NEC wakes from S3 but not s2idle, also if waking with a keyboard. Maybe a HW bug: expects absence of core power.

Several other cards (VL805, FL1100, uPD720202, ASM1142, ASM3142) wake from s2idle but not S3, including with a keyboard.

No difference if this patch is reverted, no difference if PCD is explicitly cleared in xhci_suspend(). And same problem in Windows. Looks like a HW deficiency in those cards.

I also ran into this problem (and applied the patch):
https://lore.kernel.org/linux-usb/[email protected]/

Actually it's quite obvious: I know for sure that some of them derive Vbus from the 12V rail, and chances are that the others also do.

Now I'm surprised that NEC and Etron cards bothered to make it work. Looks like PCIe cards aren't the best option for testing suspend.

Thanks for testing and clarifying

Adding this patch to queue

-Mathias