Re: [PATCHv3 10/12] usb: dwc3: add ULPI interface support

[Felipe Balbi]

Hi,

On Thu, Apr 30, 2015 at 01:34:22PM +0300, Heikki Krogerus wrote:
> Hi Felipe,
> 
> > > +	case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI:
> > > +		/* Soft reset here to sync the clocks */
> > > +		ret = dwc3_soft_reset(dwc);
> > 
> > you just lost all DWC3_GUSB3PIPECTL(0) and DWC3_GUSB2PHYCFG(0)
> > configurations which happened right before this switch. Essentially
> > breaking anybody who needs any of those extra bits enabled even though
> > they're not enabled by default.
> 
> Is this a problem we have with DWC3 cores older then 1.94? I don't

no, it's a problem with anybody setting any of the quirks in this
function, I'll reproduce some of the code here so we can look at it
together.


| static int dwc3_phy_setup(struct dwc3 *dwc)
| {
| 	u32 reg;
| 	int ret;
| 
| 	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
| 
| 	/*
| 	 * Above 1.94a, it is recommended to set DWC3_GUSB3PIPECTL_SUSPHY
| 	 * to '0' during coreConsultant configuration. So default value
| 	 * will be '0' when the core is reset. Application needs to set it
| 	 * to '1' after the core initialization is completed.
| 	 */
| 	if (dwc->revision > DWC3_REVISION_194A)
| 		reg |= DWC3_GUSB3PIPECTL_SUSPHY;
| 
| 	if (dwc->u2ss_inp3_quirk)
| 		reg |= DWC3_GUSB3PIPECTL_U2SSINP3OK;
| 
| 	if (dwc->req_p1p2p3_quirk)
| 		reg |= DWC3_GUSB3PIPECTL_REQP1P2P3;
| 
| 	if (dwc->del_p1p2p3_quirk)
| 		reg |= DWC3_GUSB3PIPECTL_DEP1P2P3_EN;
| 
| 	if (dwc->del_phy_power_chg_quirk)
| 		reg |= DWC3_GUSB3PIPECTL_DEPOCHANGE;
| 
| 	if (dwc->lfps_filter_quirk)
| 		reg |= DWC3_GUSB3PIPECTL_LFPSFILT;
| 
| 	if (dwc->rx_detect_poll_quirk)
| 		reg |= DWC3_GUSB3PIPECTL_RX_DETOPOLL;
| 
| 	if (dwc->tx_de_emphasis_quirk)
| 		reg |= DWC3_GUSB3PIPECTL_TX_DEEPH(dwc->tx_de_emphasis);
| 
| 	if (dwc->dis_u3_susphy_quirk)
| 		reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
| 
| 	dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);

right here we have potentially wrote quite a few quirks for the USB3
PHY...

| 	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
| 
| 	/* Select the HS PHY interface */
| 	switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) {
| 	case DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI:
| 		if (!strncmp(dwc->hsphy_interface, "utmi", 4)) {
| 			reg &= ~DWC3_GUSB2PHYCFG_ULPI_UTMI;
| 			break;
| 		} else if (!strncmp(dwc->hsphy_interface, "ulpi", 4)) {
| 			reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI;
| 			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
| 		} else {
| 			dev_warn(dwc->dev, "HSPHY Interface not defined\n");
| 
| 			/* Relying on default value. */
| 			if (!(reg & DWC3_GUSB2PHYCFG_ULPI_UTMI))
| 				break;
| 		}
| 		/* FALLTHROUGH */
| 	case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI:
| 		/* Soft reset here to sync the clocks */
| 		ret = dwc3_soft_reset(dwc);

and right here we loose those bits :-)

| 		if (ret)
| 			return ret;
| 
| 		ret = dwc3_ulpi_init(dwc);
| 		if (ret)
| 			return ret;
| 		/* FALLTHROUGH */
| 	default:
| 		break;
| 	}
| 
| 	/*
| 	 * Above 1.94a, it is recommended to set DWC3_GUSB2PHYCFG_SUSPHY to
| 	 * '0' during coreConsultant configuration. So default value will
| 	 * be '0' when the core is reset. Application needs to set it to
| 	 * '1' after the core initialization is completed.
| 	 */
| 	if (dwc->revision > DWC3_REVISION_194A)
| 		reg |= DWC3_GUSB2PHYCFG_SUSPHY;
| 
| 	if (dwc->dis_u2_susphy_quirk)
| 		reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
| 
| 	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
| 
| 	return 0;
| }

> know anything about those. If it is, then I would imagine we just need
> to soft reset here conditionally, only cores >= 1.94a, right?

That's wrong, how do you support older core with ULPI vs UTMI selection
needs ?

> With 1.94a and newer, DWC3_GUSB3PIPECTL(0) and DWC3_GUSB2PHYCFG(0)
> keep their ctx over any kind of soft reset. And any configurations
> done to them here will take affect the latest when
> dwc3_core_soft_reset() is called.

/me goes read Databook again.

You're right. You're using the soft reset bit from DCTL, that only
resets the device side, not any global register. There are two details
which you don't appear to take care of, however.

According to Table 7-82 on Databook 2.93a (page 725), bit 30 CSFTRST,
it's said that "Once this bit is cleared, the software must wait at
least 3 PHY clocks before accessing the PHY domain". Futher down is
states that "Once a new clock is selected, the PHY domain must be reset
for proper operation".

So there you go, 2 details which should be taken care of :-)

cheers

-- 
balbi

Attachment: signature.asc
Description: Digital signature