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.
> + 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.
> + 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.
> 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" ???
> @@ -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.
> 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 ?
--
balbi
Attachment:
signature.asc
Description: Digital signature
