Re: [RFC PATCH 3/4] usb: dwc3: add quirk to be compatible for AMD NL

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux