From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932828AbbA3Qag (ORCPT ); Fri, 30 Jan 2015 11:30:36 -0500 Received: from devils.ext.ti.com ([198.47.26.153]:60703 "EHLO devils.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932613AbbA3Qad (ORCPT ); Fri, 30 Jan 2015 11:30:33 -0500 Date: Fri, 30 Jan 2015 10:30:20 -0600 From: Felipe Balbi To: David Cohen CC: Felipe Balbi , Heikki Krogerus , Greg Kroah-Hartman , Baolu Lu , , , Kishon Vijay Abraham I Subject: Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY Message-ID: <20150130163020.GE15318@saruman.tx.rr.com> Reply-To: References: <20150126192337.GA13936@psi-dev26.jf.intel.com> <20150127092856.GD28539@kuha.fi.intel.com> <20150127173801.GA8441@psi-dev26.jf.intel.com> <20150128142024.GA2378@kuha.fi.intel.com> <20150128180255.GA7551@psi-dev26.jf.intel.com> <20150129141412.GA2570@kuha.fi.intel.com> <20150129162023.GF21217@saruman.tx.rr.com> <20150130092956.GE2570@kuha.fi.intel.com> <20150130161412.GD15318@saruman.tx.rr.com> <20150130162523.GC20689@psi-dev26.jf.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="maH1Gajj2nflutpK" Content-Disposition: inline In-Reply-To: <20150130162523.GC20689@psi-dev26.jf.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 --maH1Gajj2nflutpK Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jan 30, 2015 at 08:25:23AM -0800, David Cohen wrote: > On Fri, Jan 30, 2015 at 10:14:12AM -0600, Felipe Balbi wrote: > > On Fri, Jan 30, 2015 at 11:29:56AM +0200, Heikki Krogerus wrote: > > > Hi, > > >=20 > > > > > You can't really compare a bus like i2c, which can't enumerate de= vices > > > > > natively, to ULPI which can. > > > >=20 > > > > why not ? The BIOS might not need to use the PHY (or USB) at all, i= t can > > > > very well decide to never turn it on, right ? > > >=20 > > > If ULPI was seen as a bus, then no. BIOS would have definitely left > > > the PHY on. In fact, if we would have just asked the BIOS writers to > > > leave it on, they would not have any problem with that, even without > > > the bus. > >=20 > > it doesn't make sense, what you say just doesn't make sense. You're > > assuming that a) only intel writes BIOS and b) you *always* have access > > to BIOS writers. You forget that companies other than Intel make x86 > > devices too. > >=20 > > If the BIOS left the thing switched off, there's no "oh man, if only I > > had asked them to leave it on"... that's nonsense, just have the kernel > > deal with it. > >=20 > > > > > I don't agree with PM arguments if it means that we should be rea= dy to > > > > > accept loosing possibility for a generic solution in OS with a si= ngle > > > > > device like our PHY. I seriously doubt it would prevent the produ= cts > > > > > using these boards of achieving their PM requirements. But this > > > > > conversation is outside our topic. > > > >=20 > > > > we're not loosing anything. We're just considering what's the best = way > > > > to tackle that ulpi_read() inside probe(). TUSB1210 driver _has_ to= cope > > > > with situations where reset_gpio/cs_gpio are in unexpected state. S= aying > > > > we will just "fix the firmware", as if that was a simple feat, is > > > > counter-productive. > > >=20 > > > You know guys, we shouldn't always just lay down and say, "we just > > > have to accept it can be anything" or "we just have to try to prepare > > > for everything". We can influence these things, and we should. We can > >=20 > > sure Heikki, no arguments there. But the fact of the matter is that the > > product David mentioned is *already* in the market. > >=20 > > > influence these things inside our own companies before any products is > > > launched using our SoCs, and since more and more companies are > > > releasing their code into the public before their product are > > > launched, we even have a change to influence others. Lack of standards > > > does not mean we should not try to achieve consistency. > > >=20 > > > For example, now I should probable write to Documentation that "ULPI > > > PHY needs to be in condition where it's register can be accessed > > > before the interface is registered.", and I'm pretty sure it would be > > > enough to have an effect on many of the new platforms that use ULPI > > > PHYs. > >=20 > > until then, we just have to deal with current state of affairs. > >=20 > > > > > Because of the need to write to the ULPI registers, I don't think= we > > > > > should try anything else except to use ULPI bus straight away. We= 'll > > > >=20 > > > > I'll agree with this. > > > >=20 > > > > > start by making use of ULPI bus possible by adding the quirk for = BYT > > > > > (attached), which to me is perfectly OK solution. I would appreci= ate > > > > > if you gave it a review. > > > >=20 > > > > it's not perfectly ok for dwc3 to toggle PHY's GPIOs. Have the PHY > > > > driver to that. > > >=20 > > > Oh, I agree with that.. > > >=20 > > > > > diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-= pci.c > > > > > index 8d95056..53902ea 100644 > > > > > --- a/drivers/usb/dwc3/dwc3-pci.c > > > > > +++ b/drivers/usb/dwc3/dwc3-pci.c > > > > > @@ -21,6 +21,7 @@ > > > > > #include > > > > > #include > > > > > #include > > > > > +#include > > > > > =20 > > > > > #include "platform_data.h" > > > > > =20 > > > > > @@ -35,6 +36,24 @@ > > > > > =20 > > > > > static int dwc3_pci_quirks(struct pci_dev *pdev) > > > > > { > > > > > + if (pdev->vendor =3D=3D PCI_VENDOR_ID_INTEL && > > > > > + pdev->device =3D=3D PCI_DEVICE_ID_INTEL_BYT) { > > > > > + struct gpio_desc *gpio; > > > > > + > > > > > + gpio =3D gpiod_get_index(&pdev->dev, "reset", 0); > > > > > + if (!IS_ERR(gpio)) { > > > > > + gpiod_direction_output(gpio, 0); > > > > > + gpiod_set_value_cansleep(gpio, 1); > > > > > + gpiod_put(gpio); > > > > > + } > > > > > + gpio =3D gpiod_get_index(&pdev->dev, "cs", 1); > > > > > + if (!IS_ERR(gpio)) { > > > > > + gpiod_direction_output(gpio, 0); > > > > > + gpiod_set_value_cansleep(gpio, 1); > > > > > + gpiod_put(gpio); > > > > > + } > > > > > + } > > > >=20 > > > > why would you have dwc3 mess around with the PHY's gpios ? Doesn't = look > > > > very good. > > >=20 > > > ..but unfortunately we can't use the bus without it :(. We depend on > > > being able to read the vendor and product id's in the bus driver. > >=20 > > and what's the problem on doing this within PHY's probe ? The solution > > is simple: > >=20 > > tusb1210_phy_probe() > > { > > ... > >=20 > > gpiod_get(...); > > gpiod_direction_output(reset, 0); > > gpiod_set_value_cansleep(reset, 1); > >=20 > > gpiod_get(...); > > gpiod_direction_output(cs, 0); > > gpiod_set_value_cansleep(cs, 1); > >=20 > > eye =3D ulpi_read(); > >=20 > > gpiod_set_value_cansleep(cs, 0); > > gpiod_put(cs); > >=20 > > gpiod_set_value_cansleep(reset, 0); > > gpiod_put(reset); > >=20 > > ... > >=20 > > return 0; > > } > >=20 > > This will have no effect on devices where PHY is already turned on and > > will cope with the device David mentioned. If, however, there's a way to > > get that eye diagram optimization without needing a ulpi_read() that's > > *even* better, otherwise, above should fine in all cases. >=20 > A comment here. The ulpi_read() works for eye diagram only and only if > there is no reset on PHY between boot and probe. It is assuming the BIOS > set the correct value and then we just save it right away. As soon as we > reset the PHY this value is lost. So the GPIO toggling breaks the > current eye diagram request proposal. IMHO this should really come from > _DSD which will maintain the value regardless to what happens with phy. fair enough. --=20 balbi --maH1Gajj2nflutpK Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUy7GcAAoJEIaOsuA1yqREI7AP/jKSjPyVwJojn0hc1NdqEdaq urkjkkp0gMlIUMUmlg9jBAqj9Fq1l4Y4CyAzYIb6cZyqVcXrt5YjLquz69DtndLF dz6W9VXQxKEXd8m26fpEttNRPHX3RYOkMPuwwH+uV/bWAo0CRADIWIoPaX5J9qzA ehQHoFk7WkGN7Wq1WnbapbDVqO+RjbF4jm4xi97xVd4HiA7+bU37fvWx48FpK3BC j4JCLDv4u+DO1kQ1cAX67Anq+Dxrm36QgLUPhWeMGG1PA5bo1xlc5bwuBbzk25dp Og203R6jri/Gesw9BvDsv+f2u93JKpOacDYq81lxIFQHf8hQlF9SIbghZHUMnvVS LXS/jJVR9EiS9COCP+IjdxteM/AAmgIBv697oBhTDgrbCFZX3oV+HKnfdL9HKE/3 CEqR2fcCmFxCITrU7wV1WanfeTLqx4Hg4APqSH+OAqki1ziO7/fCayBWrnV+HMWr BbQ3ZzaI6H+w8elTQuvTC1FcYTzFwIQHrtWfonBvLYGc3RHw2XTy//O2CRAWMRCY 4L9l9LIaxwnrcaS16PAnuItke1fLiIlhG5oWyEHffYhj2aFEhnk2lXSSpKME6Foo KMMeIuwzsD+qGqkn4baVLuPuFyjrRRhYqq8GViiIAKQDO21qjJYuq1+cTOjdeZ2t o5i7xumBgse2Hy1KJMqo =VhqB -----END PGP SIGNATURE----- --maH1Gajj2nflutpK--