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:
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");
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 <linux/usb/otg.h>
> > > #include <linux/usb/usb_phy_generic.h>
> > >
> > > +#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
--
balbi
Attachment:
signature.asc
Description: Digital signature
