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.
> > + 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.
> > + 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.
> > 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.
So should I define DWC3_AMD_NL_PLAT and other QUIRKS at
platform_data.h or create a quirks.h file?
> > @@ -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?
> > 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.
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
