From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752957AbbD3O4W (ORCPT ); Thu, 30 Apr 2015 10:56:22 -0400 Received: from arroyo.ext.ti.com ([192.94.94.40]:43557 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751513AbbD3O4R (ORCPT ); Thu, 30 Apr 2015 10:56:17 -0400 Date: Thu, 30 Apr 2015 09:54:39 -0500 From: Felipe Balbi To: Heikki Krogerus CC: Felipe Balbi , David Cohen , Greg Kroah-Hartman , Stephen Boyd , Baolu Lu , Paul Bolle , , Subject: Re: [PATCHv3 10/12] usb: dwc3: add ULPI interface support Message-ID: <20150430145439.GC1515@saruman.tx.rr.com> Reply-To: References: <20150429082113.GC25288@kuha.fi.intel.com> <1430296233-144245-3-git-send-email-heikki.krogerus@linux.intel.com> <20150429150450.GC7262@saruman.tx.rr.com> <20150430103422.GB1372@kuha.fi.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="ctP54qlpMx3WjD+/" Content-Disposition: inline In-Reply-To: <20150430103422.GB1372@kuha.fi.intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --ctP54qlpMx3WjD+/ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Thu, Apr 30, 2015 at 01:34:22PM +0300, Heikki Krogerus wrote: > Hi Felipe, >=20 > > > + case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI: > > > + /* Soft reset here to sync the clocks */ > > > + ret =3D dwc3_soft_reset(dwc); > >=20 > > 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. >=20 > 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; |=20 | reg =3D dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); |=20 | /* | * 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 |=3D DWC3_GUSB3PIPECTL_SUSPHY; |=20 | if (dwc->u2ss_inp3_quirk) | reg |=3D DWC3_GUSB3PIPECTL_U2SSINP3OK; |=20 | if (dwc->req_p1p2p3_quirk) | reg |=3D DWC3_GUSB3PIPECTL_REQP1P2P3; |=20 | if (dwc->del_p1p2p3_quirk) | reg |=3D DWC3_GUSB3PIPECTL_DEP1P2P3_EN; |=20 | if (dwc->del_phy_power_chg_quirk) | reg |=3D DWC3_GUSB3PIPECTL_DEPOCHANGE; |=20 | if (dwc->lfps_filter_quirk) | reg |=3D DWC3_GUSB3PIPECTL_LFPSFILT; |=20 | if (dwc->rx_detect_poll_quirk) | reg |=3D DWC3_GUSB3PIPECTL_RX_DETOPOLL; |=20 | if (dwc->tx_de_emphasis_quirk) | reg |=3D DWC3_GUSB3PIPECTL_TX_DEEPH(dwc->tx_de_emphasis); |=20 | if (dwc->dis_u3_susphy_quirk) | reg &=3D ~DWC3_GUSB3PIPECTL_SUSPHY; |=20 | dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); right here we have potentially wrote quite a few quirks for the USB3 PHY... | reg =3D dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); |=20 | /* 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 &=3D ~DWC3_GUSB2PHYCFG_ULPI_UTMI; | break; | } else if (!strncmp(dwc->hsphy_interface, "ulpi", 4)) { | reg |=3D DWC3_GUSB2PHYCFG_ULPI_UTMI; | dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); | } else { | dev_warn(dwc->dev, "HSPHY Interface not defined\n"); |=20 | /* 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 =3D dwc3_soft_reset(dwc); and right here we loose those bits :-) | if (ret) | return ret; |=20 | ret =3D dwc3_ulpi_init(dwc); | if (ret) | return ret; | /* FALLTHROUGH */ | default: | break; | } |=20 | /* | * 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 |=3D DWC3_GUSB2PHYCFG_SUSPHY; |=20 | if (dwc->dis_u2_susphy_quirk) | reg &=3D ~DWC3_GUSB2PHYCFG_SUSPHY; |=20 | dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); |=20 | return 0; | } > know anything about those. If it is, then I would imagine we just need > to soft reset here conditionally, only cores >=3D 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 --=20 balbi --ctP54qlpMx3WjD+/ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVQkIvAAoJEIaOsuA1yqREW70P/0IgL81lTy1DBhaXwvPN0d/7 OqzfIITEZSoeGUOsheV/Jtm11LvkFnBAFs8+oolCt1M1MWZJrHxENkfB14PTB9RF l3cBnb/Yu6r0nNw87kLDC17dD/iIlawxft32rA5gk75jWj3p13zDZSkAr1O+22Cz BvzUCp/rrTCeXgbCnSnjcggJKoH2HXMustF1IUHwSOxoCMnddwqGrk36jQw7F8Cp xHWV5fQxSus0lOcLZyu90DXme+eRWlbLPxk4NoD3pIQL5memcqdoOKSaX6edG/gN 4soFpoC+2qjDvO1URZ0Y/Ee+BB6p6xNrBlQNsPhX1lQrUuEv2WBQ2BS4JjVRZEX0 INfNYyUtfGNxZCHB+7J+YeFpvQkWi0WVCZqtbIYN0kvoEaHkj1JET76bRGjX0C56 vDditK83k5HVnHXKCCvuke6kLNsF+2fGyTmWsy12ATq8F5RFz/n4LtkvDRWx4WdR qrBl7egkmbEFYbxsB5sPLrnHp30HcMZKaMDb/fudxT2cEh9LQ9ss5wlV/kXP0KqI keYeOkBzeeoGRwRd5YPG+Yz5R88BJj74mvYUDVm6+J0L0JUNh/kC9NneV6yA8q7t HqE4UDgl7jAiSq0UeqxmY6+3bYIdKtuHgCmc5DxpNEm0AJFmKCFtteWguK/Y+lTB GRncAE3EM6rYR30Rb1mm =isSd -----END PGP SIGNATURE----- --ctP54qlpMx3WjD+/--