From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759212AbbA3QOM (ORCPT ); Fri, 30 Jan 2015 11:14:12 -0500 Received: from comal.ext.ti.com ([198.47.26.152]:40089 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751023AbbA3QOK (ORCPT ); Fri, 30 Jan 2015 11:14:10 -0500 Date: Fri, 30 Jan 2015 10:14:12 -0600 From: Felipe Balbi To: Heikki Krogerus CC: Felipe Balbi , David Cohen , Greg Kroah-Hartman , Baolu Lu , , , Kishon Vijay Abraham I Subject: Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY Message-ID: <20150130161412.GD15318@saruman.tx.rr.com> Reply-To: References: <20150124235811.GA24665@psi-dev26.jf.intel.com> <20150126125503.GB28539@kuha.fi.intel.com> <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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="3Pql8miugIZX0722" Content-Disposition: inline In-Reply-To: <20150130092956.GE2570@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 --3Pql8miugIZX0722 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 devices > > > natively, to ULPI which can. > >=20 > > why not ? The BIOS might not need to use the PHY (or USB) at all, it 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. 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. 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. > > > I don't agree with PM arguments if it means that we should be ready to > > > accept loosing possibility for a generic solution in OS with a single > > > device like our PHY. I seriously doubt it would prevent the products > > > 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. Saying > > 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 sure Heikki, no arguments there. But the fact of the matter is that the product David mentioned is *already* in the market. > 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. until then, we just have to deal with current state of affairs. > > > 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 appreciate > > > 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. and what's the problem on doing this within PHY's probe ? The solution is simple: tusb1210_phy_probe() { ... gpiod_get(...); gpiod_direction_output(reset, 0); gpiod_set_value_cansleep(reset, 1); gpiod_get(...); gpiod_direction_output(cs, 0); gpiod_set_value_cansleep(cs, 1); eye =3D ulpi_read(); gpiod_set_value_cansleep(cs, 0); gpiod_put(cs); gpiod_set_value_cansleep(reset, 0); gpiod_put(reset); ... return 0; } 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. cheers --=20 balbi --3Pql8miugIZX0722 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUy63UAAoJEIaOsuA1yqRE2fIP/04xAMQT2A4cVjIV5gt8B7UQ bDyuIP1BxrPttoVf76U4DRLJAVhNj+A+6LprAaNTDSIdc92isMr1SdvTTnzMSJtK gfWoQ76gAYBvmFvyzcoveT0c6uzgOqRuGbTefZd1AtbKR6GFjaH/WwXq6/O+CrZY Vj5s8tVdM8HfZnqnVVGPcjINxrAD8frksqmL5r9naXkt7yvnptuDBR+om176gsZ7 n7ab1t06tFrgfjJpdnIRUmpfBr2FgbYnfwEAtE4LRpHa8UGammYUC6+6jXoGLYka WF/5oUdEQu2byg4xCJ/WG0/tzRkdmKIlbJ4yCaF40zDA8SM3tnvDPZ7QyoHGzTV1 xv4WWvKYOTmylSpWV17HGKnwXeVBZGG20l7CuRx9ndlz5kCKWkiThsfTeMSew4WF 4inlViYwrgOiBYRnxGHSfxm0hUKEFuMti6sY2hr9fxH2ISgJfYwHoJ6JybK2a9G5 xGl1FGq0III9YTtufTiEJ8a7YuFEC8mWVgdkMuO+aujaKdFNZNSNb8x5NcILSvAz 7sC2fqGSA7ds63rYo0Hlmwx04xFNCFl/pMY9fxX+dR3ow6bej5lKQElRdgIGqViQ PWjlJdZIWpAVWM1LRFdCegXGxyyv8YrRIV/g/sFLsk8/Wiz2zihj5s21LlYeedwx tPCkqNFlBUuVo6zAMlUE =sQSp -----END PGP SIGNATURE----- --3Pql8miugIZX0722--