Hi Benson, Please see responses inline. -----Original Message----- From: bleung@xxxxxxxxxx [mailto:bleung@xxxxxxxxxx] On Behalf Of Benson Leung Sent: Monday, July 13, 2015 3:45 PM To: Mani, Rajmohan Cc: Nyman, Mathias; linux-usb@xxxxxxxxxxxxxxx; ejcaruso@xxxxxxxxxxxx Subject: Re: [PATCH] xhci: Workaround to get D3 working in Intel xHCI Thanks Rajmohan. Comments inline. On Thu, Jul 9, 2015 at 6:17 PM, <rajmohan.mani@xxxxxxxxx> wrote: > From: Rajmohan Mani <rajmohan.mani@xxxxxxxxx> > > The xHCI in Intel CherryView / Braswell Platform requires a driver > workaround to get xHCI D3 working. Without this workaround, xHCI might > not enter D3. > > Workaround is to configure SSIC PORT as "unused" before D3 entry and > "used" after D3 exit. This is done through a vendor specific register > (PORT2_SSIC_CONFIG_REG2 at offset 0x883c), in xhci suspend / resume > callbacks. > > Verified xHCI D3 works fine in CherryView / Braswell platform. > > Signed-off-by: Rajmohan Mani <rajmohan.mani@xxxxxxxxx> > --- > drivers/usb/host/xhci-pci.c | 40 > +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 37 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c > index 91260b3..7a69a12 100644 > --- a/drivers/usb/host/xhci-pci.c > +++ b/drivers/usb/host/xhci-pci.c > @@ -27,6 +27,10 @@ > #include "xhci.h" > #include "xhci-trace.h" > > +#define PORT2_SSIC_CONFIG_REG2 0x883c > +#define PROG_DONE (1 << 30) > +#define SSIC_PORT_UNUSED (1 << 31) > + > /* Device for a quirk */ > #define PCI_VENDOR_ID_FRESCO_LOGIC 0x1b73 > #define PCI_DEVICE_ID_FRESCO_LOGIC_PDK 0x1000 @@ -167,14 +171,44 @@ > static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) > } > > /* > + * In some Intel xHCI controllers, in order to get D3 working, > + * through a vendor specific SSIC CONFIG register at offset 0x883c, > + * SSIC PORT need to be marked as "unused" before putting xHCI > + * into D3. After D3 exit, the SSIC port need to be marked as "used". > + * Without this change, xHCI might not enter D3 state. > * Make sure PME works on some Intel xHCI controllers by writing 1 to clear > * the Internal PME flag bit in vendor specific PMCTRL register at offset 0x80a4 > */ > -static void xhci_pme_quirk(struct xhci_hcd *xhci) > +static void xhci_pme_quirk(struct usb_hcd *hcd, bool suspend) Is the function name here still descriptive of the quirk in question? It seems like the SSIC change is a separate workaround entirely from the PME quirk that existed here previously, but you're just reusing this function because it's conveniently called at suspend/resume. Raj: Yes. SSIC change is related to the PME quirk. It is just that it is needed only for CherryView XHCI. Without this SSIC change, a PME event would wake XHCI from D3. > { > + struct xhci_hcd *xhci = hcd_to_xhci(hcd); > + struct pci_dev *pdev = to_pci_dev(hcd->self.controller); > u32 val; > void __iomem *reg; > > + if (pdev->vendor == PCI_VENDOR_ID_INTEL && > + pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) > + { I would prefer you didn't do this here. Instead, we should keep all of the vendor/device matching in one place, namely xhci_pci_quirks(). You should create a new quirk name for this ssic D3 workaround, and match it only to PCI_VENDOR_ID_INTEL && PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI in xhci_pci_quirks, for now. Also, this block should be in a separate function from xhci_pme_quirk. Raj: I implemented the original patch exactly the way you mentioned that would i) use a new quirk ii) do the vendor / device check within xhci_pci_quirks() and iii) do this in a separate function. When I sent this out internally to Mathias Nyman, he suggested that it would be simpler / cleaner, if we can consolidate this new quirk into the existing PME quirk, since this is also related to the PME quirk. He also mentioned that we are running out of quirk bits. In response to his comments, I came up with this patch. > + > + reg = (void __iomem *) xhci->cap_regs + > + PORT2_SSIC_CONFIG_REG2; > + > + /* Notify SSIC that SSIC profile programming is not done */ > + val = readl(reg) & ~PROG_DONE; > + writel(val, reg); > + > + /* Mark SSIC port as unused(suspend) or used(resume) */ > + val = readl(reg); > + if (suspend) > + val |= SSIC_PORT_UNUSED; > + else > + val &= ~SSIC_PORT_UNUSED; > + writel(val, reg); > + > + /* Notify SSIC that SSIC profile programming is done */ > + val = readl(reg) | PROG_DONE; > + writel(val, reg); > + readl(reg); > + } > + > reg = (void __iomem *) xhci->cap_regs + 0x80a4; > val = readl(reg); > writel(val | BIT(28), reg); > @@ -301,7 +335,7 @@ static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup) > pdev->no_d3cold = true; > > if (xhci->quirks & XHCI_PME_STUCK_QUIRK) > - xhci_pme_quirk(xhci); > + xhci_pme_quirk(hcd, true); > > return xhci_suspend(xhci, do_wakeup); } @@ -334,7 +368,7 @@ > static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated) > usb_enable_intel_xhci_ports(pdev); > > if (xhci->quirks & XHCI_PME_STUCK_QUIRK) > - xhci_pme_quirk(xhci); > + xhci_pme_quirk(hcd, false); > > retval = xhci_resume(xhci, hibernated); > return retval; > -- > 1.9.1 > -- Benson Leung Software Engineer, Chrom* OS bleung@xxxxxxxxxxxx ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
