On 21/05/18 09:23, Mathias Nyman wrote:
> On 18.05.2018 19:29, Marc Zyngier wrote:
>> Some Renesas controllers get into a weird state if they are reset while
>> programmed with 64bit addresses (they will preserve the top half of the
>> address in internal, non visible registers).
>>
>> You end up with half the address coming from the kernel, and the other
>> half coming from the firmware.
>
> Should those registers be zeroed in resume from hibernate where we also
> reset the host controller?
That's a very good point. Given that this controller also has the
XHCI_RESET_ON_RESUME quick, it is always treated as coming out of
hibernate (how this thing ended up in production is beyond me).
I'll place another stick of dynamite at that level too.
>> Also, changing the programming leads to extra accesses even if the
>> controller is supposed to be halted. The controller ends up with a fatal
>> fault, and is then ripe for being properly reset. On the flip side,
>> this is completely unsafe if the defvice isn't behind an IOMMU, so
>> we have to make sure that this is the case. Can you say "broken"?
>>
>> This is an alternative method to the one introduced in 8466489ef5ba
>> ("xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue"),
>> which will subsequently be removed.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>> drivers/usb/host/xhci-pci.c | 8 ++++--
>> drivers/usb/host/xhci.c | 59 +++++++++++++++++++++++++++++++++++++++++++++
>> drivers/usb/host/xhci.h | 1 +
>> 3 files changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> index 85ffda85f8ab..e0a0a12871e2 100644
>> --- a/drivers/usb/host/xhci-pci.c
>> +++ b/drivers/usb/host/xhci-pci.c
>> @@ -196,11 +196,15 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>> xhci->quirks |= XHCI_BROKEN_STREAMS;
>> }
>> if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
>> - pdev->device == 0x0014)
>> + pdev->device == 0x0014) {
>> xhci->quirks |= XHCI_TRUST_TX_LENGTH;
>> + xhci->quirks |= XHCI_ZERO_64B_REGS;
>> + }
>> if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
>> - pdev->device == 0x0015)
>> + pdev->device == 0x0015) {
>> xhci->quirks |= XHCI_RESET_ON_RESUME;
>> + xhci->quirks |= XHCI_ZERO_64B_REGS;
>> + }
>> if (pdev->vendor == PCI_VENDOR_ID_VIA)
>> xhci->quirks |= XHCI_RESET_ON_RESUME;
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 8dba26d3de07..07272d1ce32a 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -4921,6 +4921,65 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>> if (retval)
>> return retval;
>>
>> + /*
>> + * Some Renesas controllers get into a weird state if they are
>> + * reset while programmed with 64bit addresses (they will preserve
>> + * the top half of the address in internal, non visible
>> + * registers). You end up with half the address coming from the
>> + * kernel, and the other half coming from the firmware. Also,
>> + * changing the programming leads to extra accesses even if the
>> + * controller is supposed to be halted. The controller ends up with
>> + * a fatal fault, and is then ripe for being properly reset.
>> + *
>> + * Special care is taken to only apply this if the device is behind
>> + * an iommu. Doing anything when there is no iommu is definitely
>> + * unsafe...
>> + */
>> + if ((xhci->quirks & XHCI_ZERO_64B_REGS) && dev->iommu_group) {
>> + u64 val;
>> + int i;
>> +
>> + xhci_info(xhci,
>> + "Zeroing 64bit base registers, expecting fault\n");
>> +
>> + /* Clear HSEIE so that faults do not get signaled */
>> + val = readl(&xhci->op_regs->command);
>> + val &= ~CMD_HSEIE;
>> + writel(val, &xhci->op_regs->command);
>> +
>> + /* Clear HSE (aka FATAL) */
>> + val = readl(&xhci->op_regs->status);
>> + val |= STS_FATAL;
>> + writel(val, &xhci->op_regs->status);
>> +
>> + /* Now zero the registers, and brace for impact */
>> + val = xhci_read_64(xhci, &xhci->op_regs->dcbaa_ptr);
>> + if (upper_32_bits(val))
>> + xhci_write_64(xhci, 0, &xhci->op_regs->dcbaa_ptr);
>> + val = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
>> + if (upper_32_bits(val))
>> + xhci_write_64(xhci, 0, &xhci->op_regs->cmd_ring);
>> +
>> + for (i = 0; i < HCS_MAX_INTRS(xhci->hcs_params1); i++) {
>> + struct xhci_intr_reg __iomem *ir;
>> +
>> + ir = &xhci->run_regs->ir_set[i];
>> + val = xhci_read_64(xhci, &ir->erst_base);
>> + if (upper_32_bits(val))
>> + xhci_write_64(xhci, 0, &ir->erst_base);
>> + val= xhci_read_64(xhci, &ir->erst_dequeue);
>> + if (upper_32_bits(val))
>> + xhci_write_64(xhci, 0, &ir->erst_dequeue);
>> + }
>> +
>> + /* Wait for the fault to appear. It will be cleared on reset */
>> + retval = xhci_handshake(&xhci->op_regs->status,
>> + STS_FATAL, STS_FATAL,
>> + XHCI_MAX_HALT_USEC);
>> + if (!retval)
>> + xhci_info(xhci, "Fault detected\n");
>> + }
>
> Small thing, but could this quirk code be moved to it's own function?
> xhci_gen_setup() is growing quite big, and harder to follow the big picture.
Given the above, it definitely makes sense.
> Otherwise it looks good to me
Thanks, I'll repost the series.
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
