On Fri, Sep 26, 2014 at 09:35:21AM -0500, Felipe Balbi wrote:
> 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 ...
> "
>
Got it, so faulty PHY you meant that some special PHYs didn't not meet
the standards someplace.
For simulation board, we used vendor provided PHY. But on SOC, we
will use AMD PHY. I will re-check again to confirm which workarounds
need on simulation board and which workarounds need on SOC.
> 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.
>
Yes, but currently, I needn't write AMD own phy driver. There isn't
any requirement from HW side to program the phy register. So I used
NOP USB transceiver driver till now.
> 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.
I will provide the boot logs to you. Actually, I already did the
gadget mass storage and gadget zero testing with my patches under 3.14
before.
> 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.
>
OK, will post the logs to you.
> 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.
>
OK, I got it. Will rebase my patches to testing/next.
> > > > + 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 :-)
>
OK, I got it. Will update it in V2.
> > 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 :-)
Thanks.
>
> > > > 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
Felipe, it's pleasure to leverage your dwc3 ip driver on AMD platform
for me. Thanks to support. :)
Thanks,
Rui
--
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
