From patchwork Fri Sep 26 14:35:21 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Felipe Balbi X-Patchwork-Id: 38000 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-la0-f72.google.com (mail-la0-f72.google.com [209.85.215.72]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 2C41420F2E for ; Fri, 26 Sep 2014 14:35:45 +0000 (UTC) Received: by mail-la0-f72.google.com with SMTP id mc6sf2831231lab.11 for ; Fri, 26 Sep 2014 07:35:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:date:from:to:cc:subject:message-id :reply-to:references:mime-version:content-type:content-disposition :in-reply-to:user-agent:sender:precedence:list-id:x-original-sender :x-original-authentication-results:mailing-list:list-post:list-help :list-archive:list-unsubscribe; bh=ZFuisRRUiFufV4k5ieNbExBLgXEa0nAn3VUs2wZcM4E=; b=Tl5hCA5gFYomYSsYSd0WXw0YdZmJsll80wbYFy2LCAPy6nU2tx6vpAU7grUs3a+vTC 02LdhEamFDZoWbg0g3nwo4y+uAvB2QKHUyKf5h18CZoRSZUFPK9mTBMH188X7NALDB23 ATsbfi/Jefdh6QUQSqgkxWl3SjPjQjRJKCTNNyNqhmEdyvjWTZxnEWm1r9lIcnctH0Jh Y18/JeFQY9Ekv5vkvfRz7BgSY7lHPZ51IbeBDDhUx3ZV2VbiX2JLjVDlqugYTtHVRbFM wSdSoDsTnINEe5kg3xBeZMkTq7DcqmUFG40VKsiirpdxNH9Vo0IWXfatBPsUjei6zzUm gDzQ== X-Gm-Message-State: ALoCoQkFgLzpusxXwmQVBE5fJoOPff7Z29TBWkr/2+KbEH1fTjK2w4aMjVHDRKSFw9BGjBT/a6k7 X-Received: by 10.180.160.169 with SMTP id xl9mr6218303wib.7.1411742143956; Fri, 26 Sep 2014 07:35:43 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.88.103 with SMTP id bf7ls390507lab.11.gmail; Fri, 26 Sep 2014 07:35:43 -0700 (PDT) X-Received: by 10.152.22.100 with SMTP id c4mr2159052laf.0.1411742143796; Fri, 26 Sep 2014 07:35:43 -0700 (PDT) Received: from mail-lb0-f181.google.com (mail-lb0-f181.google.com [209.85.217.181]) by mx.google.com with ESMTPS id i4si7369849lab.115.2014.09.26.07.35.43 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Fri, 26 Sep 2014 07:35:43 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.181 as permitted sender) client-ip=209.85.217.181; Received: by mail-lb0-f181.google.com with SMTP id n15so285381lbi.26 for ; Fri, 26 Sep 2014 07:35:43 -0700 (PDT) X-Received: by 10.112.130.226 with SMTP id oh2mr3333858lbb.100.1411742142906; Fri, 26 Sep 2014 07:35:42 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.130.169 with SMTP id of9csp71320lbb; Fri, 26 Sep 2014 07:35:41 -0700 (PDT) X-Received: by 10.68.139.232 with SMTP id rb8mr31773415pbb.20.1411742140651; Fri, 26 Sep 2014 07:35:40 -0700 (PDT) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id pk5si9547057pdb.182.2014.09.26.07.35.40 for ; Fri, 26 Sep 2014 07:35:40 -0700 (PDT) Received-SPF: none (google.com: linux-usb-owner@vger.kernel.org does not designate permitted sender hosts) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755080AbaIZOfe (ORCPT + 3 others); Fri, 26 Sep 2014 10:35:34 -0400 Received: from bear.ext.ti.com ([192.94.94.41]:54455 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755770AbaIZOfb (ORCPT ); Fri, 26 Sep 2014 10:35:31 -0400 Received: from dflxv15.itg.ti.com ([128.247.5.124]) by bear.ext.ti.com (8.13.7/8.13.7) with ESMTP id s8QEZPSW022562; Fri, 26 Sep 2014 09:35:25 -0500 Received: from DLEE70.ent.ti.com (dlemailx.itg.ti.com [157.170.170.113]) by dflxv15.itg.ti.com (8.14.3/8.13.8) with ESMTP id s8QEZP9H002632; Fri, 26 Sep 2014 09:35:25 -0500 Received: from dflp32.itg.ti.com (10.64.6.15) by DLEE70.ent.ti.com (157.170.170.113) with Microsoft SMTP Server id 14.3.174.1; Fri, 26 Sep 2014 09:35:24 -0500 Received: from localhost (ileax41-snat.itg.ti.com [10.172.224.153]) by dflp32.itg.ti.com (8.14.3/8.13.8) with ESMTP id s8QEZOxC024128; Fri, 26 Sep 2014 09:35:24 -0500 Date: Fri, 26 Sep 2014 09:35:21 -0500 From: Felipe Balbi To: Huang Rui CC: Felipe Balbi , Alan Stern , Greg Kroah-Hartman , Subject: Re: [RFC PATCH 3/4] usb: dwc3: add quirk to be compatible for AMD NL Message-ID: <20140926143521.GA26227@saruman> Reply-To: References: <1411629707-7131-1-git-send-email-ray.huang@amd.com> <1411629707-7131-4-git-send-email-ray.huang@amd.com> <20140925145032.GE28045@saruman> <20140926085025.GB23947@hr-slim.amd.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20140926085025.GB23947@hr-slim.amd.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-usb-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: linux-usb@vger.kernel.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: balbi@ti.com X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.181 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , On Fri, Sep 26, 2014 at 04:50:26PM +0800, Huang Rui wrote: > On Thu, Sep 25, 2014 at 09:50:32AM -0500, Felipe Balbi wrote: > > Hi, > > > > On Thu, Sep 25, 2014 at 03:21:46PM +0800, Huang Rui wrote: > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > index b0f4d52..6138c5d 100644 > > > --- a/drivers/usb/dwc3/core.c > > > +++ b/drivers/usb/dwc3/core.c > > > @@ -115,6 +115,25 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc) > > > } > > > > > > /** > > > + * dwc3_core_nl_set_pipe3 - Configure USB3 PHY Interface for NL > > > + * @dwc: Pointer to our controller context structure > > > + */ > > > +static void dwc3_core_nl_set_pipe3(struct dwc3 *dwc) > > > +{ > > > + u32 reg = 0; > > > + > > > + reg |= DWC3_GUSB3PIPECTL_U2SSINP3OK | DWC3_GUSB3PIPECTL_UX_EXITINPX > > > + | DWC3_GUSB3PIPECTL_UX_EXITINPX | DWC3_GUSB3PIPECTL_U1U2EXITFAIL > > > + | DWC3_GUSB3PIPECTL_DEPOCHANGE | DWC3_GUSB3PIPECTL_RX_DETOPOLL; > > > > UX_EXITINPX is supposed to be used with a faulty PHY, I need to see an > > erratum before I can accept it. You have also duplicated the bit in this > > initialization. > > > > U1U2EXITFAIL -> also a workaround bit and I need to see an erratum. > > > > RX_DETOPOLL -> it seems like it's safe to leave this one out as it can > > only be proven to work with this bit after going through full USB > > certification. > > > > What do you mean of the faulty PHY? > We would use AMD PHY to talk with DWC3 controller. Look at the description of each of those bits and you'll see it mentions they're supposed to be used for workarounds. Here's UX_EXIT_IN_PX, as an example: " This bit is added for SS PHY workaround where SS PHY ... " It's alright that AMD PHY needs this bit, but then, let's get confirmation from IP/SoC/SilVal team and add a proper comment stating why we need them. This is so we don't forget that $version of AMD's PHY needs workarounds for A, B, and C silicon errata. Also, I'd have to ask you to provide full boot logs of your platform booting with my testing/next (where all the latest patches are) plus your patches. Then, load g_mass_storage with a backing storage of your choice and run my msc.c/msc.sh tools which you can get from [1] and [2]; post the logs for that too. Last, but not least, please USB30CV (chapter 9 and Link Layer test, at least) just so we know there isn't anything new with your version of the core, which I suppose is 2.80a, based on the LPM Errata bits. This is just because I don't have access to the HW myself, so I can't verify your patches. One thing I can tell you, with my testing/next, dwc3 is really stable. I have every test passing except for Halt Endpoint which I'm debugging right now. > > > + reg |= DWC3_GUSB3PIPECTL_DEP1P2P3(1) | DWC3_GUSB3PIPECTL_TX_DEEPH(1); > > > > DEP1P2P3 and DEP0CHANGE seem to be required only for faulty PHYs too, I > > need to see an erratum. > > > > > + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); > > > + > > > + mdelay(100); > > > +} > > > + > > > +/** > > > * dwc3_free_one_event_buffer - Frees one event buffer > > > * @dwc: Pointer to our controller context structure > > > * @evt: Pointer to event buffer to be freed > > > @@ -412,9 +431,19 @@ static int dwc3_core_init(struct dwc3 *dwc) > > > if (ret) > > > goto err0; > > > > > > + if (dwc->quirks & DWC3_AMD_NL_PLAT) > > > + dwc3_core_nl_set_pipe3(dwc); > > > + > > > reg = dwc3_readl(dwc->regs, DWC3_GCTL); > > > + > > > reg &= ~DWC3_GCTL_SCALEDOWN_MASK; > > > - reg &= ~DWC3_GCTL_DISSCRAMBLE; > > > + > > > + if (dwc->quirks & DWC3_AMD_NL_PLAT) { > > > + reg |= DWC3_GCTL_DISSCRAMBLE; > > > > you're disabling scrambling ? What about EMI ? Why doesn't your device > > work with scrambling ? I need to see an erratum before I can accept > > this. > > > > Sorry, disabling scrambling is workaround for debugging the temporary > simulation board, needn't be set for the SoC chip. Will update in v2. oh, alright. Then let's not merge this in mainline. I guess I have an idea which simulation board you're using :-) > > > + reg |= DWC3_GCTL_U2EXIT_LFPS; > > > > hmm, seems like this bit was added due to a faulty host which was found. > > I need to see an erratum before I can accept this. > > > > > + reg |= DWC3_GCTL_GBLHIBERNATIONEN; > > > > looks like this should always be set for all versions of the core > > configured with hibernation. > > > > yes, amd nl chip will support hibernation. in that case, we do this: that'll work for everybody with hibernation enabled in coreConsultant. > > > diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c > > > index cebbd01..7f471ff 100644 > > > --- a/drivers/usb/dwc3/dwc3-pci.c > > > +++ b/drivers/usb/dwc3/dwc3-pci.c > > > @@ -25,6 +25,9 @@ > > > #include > > > #include > > > > > > +#include "platform_data.h" > > > +#include "core.h" > > > > you should never need to include "core.h", why are you including > > "core.h" ??? > > > > Because I defined DWC3_AMD_NL_PLAT in dwc3 struture at core.h. I think > this quirk might be included on most of the source files like core.c, > gadget.c. If I added into platform_data.h, it would make these source > files include "platform_data.h" either. So I add DWC3_AMD_NL_PLAT into > core.h. but you're using a platform_data, right ? And you're passing the quirk through platform data, right ? It just makes sense to define all that inside platform_data.h :-) > So should I define DWC3_AMD_NL_PLAT and other QUIRKS at > platform_data.h or create a quirks.h file? platform_data.h makes sense to me :-) > > > @@ -102,6 +105,9 @@ static int dwc3_pci_probe(struct pci_dev *pci, > > > struct dwc3_pci *glue; > > > int ret; > > > struct device *dev = &pci->dev; > > > + struct dwc3_platform_data dwc3_pdata; > > > + > > > + memset(&dwc3_pdata, 0x00, sizeof(dwc3_pdata)); > > > > > > glue = devm_kzalloc(dev, sizeof(*glue), GFP_KERNEL); > > > if (!glue) > > > @@ -148,6 +154,14 @@ static int dwc3_pci_probe(struct pci_dev *pci, > > > > > > pci_set_drvdata(pci, glue); > > > > > > + if (pci->vendor == PCI_VENDOR_ID_AMD && > > > + pci->device == PCI_DEVICE_ID_AMD_NL) > > > + dwc3_pdata.quirks |= DWC3_AMD_NL_PLAT; > > > > this looks wrong. It looks like you need several quirk bits for each of > > the workarounds you need. Having a single bit for "my device" is wrong > > and if your next device fixes one or two of these errata, you'd still > > have to introduce a new "my new device" bit, instead of just clearing > > the ones which were fixed. > > > > I got it, so do you mean: > if I set "disabling scrambling" workaround, I should use a macro as > DWC3_DISSCRAMBING_QUIRK to present this specific setting. Then use the > Deivce ID to enable these kinds of the quirks I need, is that right? perfect :-) > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > > index 0fcc0a3..8277065 100644 > > > --- a/drivers/usb/dwc3/gadget.c > > > +++ b/drivers/usb/dwc3/gadget.c > > > @@ -2635,6 +2635,7 @@ static irqreturn_t dwc3_interrupt(int irq, void *_dwc) > > > */ > > > int dwc3_gadget_init(struct dwc3 *dwc) > > > { > > > + u32 reg; > > > int ret; > > > > > > dwc->ctrl_req = dma_alloc_coherent(dwc->dev, sizeof(*dwc->ctrl_req), > > > @@ -2689,6 +2690,13 @@ int dwc3_gadget_init(struct dwc3 *dwc) > > > if (ret) > > > goto err4; > > > > > > + if (dwc->quirks & DWC3_AMD_NL_PLAT) { > > > + reg = dwc3_readl(dwc->regs, DWC3_DCTL); > > > + reg |= DWC3_DCTL_LPM_ERRATA(0xf); > > > > weird, why would Synopsys put this here ? It seems like this is only > > useful when LPM Errata is enabled and that's, apparently, a host-side > > thing. > > > > Paul, can you comment ? > > > > These bits are required by HW. I am asking them, will let you know > why. Alright, thanks. What we need is a way for detecting that LPM Errata is enabled, rather than checking if we're running on AMD :-) Aparently we can only check for LPM Errata Enabled through an XHCI register; kinda weird. cheers and happy hacking [1] https://gitorious.org/usb/usb-tools/source/7eb7ef21de6cd124e0e0d0e7df9ddfff0e2f548e:msc.c [2] https://gitorious.org/usb/usb-tools/source/7eb7ef21de6cd124e0e0d0e7df9ddfff0e2f548e:msc.sh diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 4d4e854..584dcde 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -449,6 +449,7 @@ static int dwc3_core_init(struct dwc3 *dwc) case DWC3_GHWPARAMS1_EN_PWROPT_HIB: /* enable hibernation here */ dwc->nr_scratch = DWC3_GHWPARAMS4_HIBER_SCRATCHBUFS(hwparams4); + reg |= DWC3_GCTL_GBLHIBERNATIONEN; break; default: dev_dbg(dwc->dev, "No power optimization available\n");