From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755548AbbA2QUg (ORCPT ); Thu, 29 Jan 2015 11:20:36 -0500 Received: from comal.ext.ti.com ([198.47.26.152]:44907 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753881AbbA2QUf (ORCPT ); Thu, 29 Jan 2015 11:20:35 -0500 Date: Thu, 29 Jan 2015 10:20:23 -0600 From: Felipe Balbi To: Heikki Krogerus CC: David Cohen , Felipe Balbi , Greg Kroah-Hartman , Baolu Lu , , , Kishon Vijay Abraham I Subject: Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY Message-ID: <20150129162023.GF21217@saruman.tx.rr.com> Reply-To: References: <1422025978-178336-1-git-send-email-heikki.krogerus@linux.intel.com> <1422025978-178336-9-git-send-email-heikki.krogerus@linux.intel.com> <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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="WlEyl6ow+jlIgNUh" Content-Disposition: inline In-Reply-To: <20150129141412.GA2570@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 --WlEyl6ow+jlIgNUh Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi On Thu, Jan 29, 2015 at 04:14:12PM +0200, Heikki Krogerus wrote: > > > > > > Can you share how tusb1210 is connected on the platform you're = using as > > > > > > test for this patch? I don't think this driver would work relia= bly with > > > > > > this device: > > > > > > http://liliputing.com/2014/11/trekstor-launches-first-android-t= ablet-based-intels-irda-reference-design.html > > > > >=20 > > > > > The only reason why that board doesn't work is because of very mu= ch > > > > > Baytrail-CR specific problems. These are are two issues, but the = first > > > >=20 > > > > That's not BYT-CR specific problems. That's just dwc3 and tusb1210 > > > > interacting as they're expecting to. > > > >=20 > > > > > one is critical for getting it working. Both will be handled, but > > > > > separately from this set: > > > > >=20 > > > > > 1) The firmware leaves the PHY in reset, forcing us to enable it > > > > > somehow in OS before accessing ulpi. Unless we can get a firmware= fix > > > > > for that (it's starting to look like it's not going to happen; pl= ease > > > > > correct me if you know something else!), we need to add a quirk f= or > > > > > Baytrails (attached), which is probable still OK. But IMO this re= ally > > > > > should be fixed in the firmware. > > > >=20 > > > > It seems you're expecting the PHY to be fully operational in order = to > > > > probe it. That's wrong assumption. BYT-CR's BIOS is doing nothing w= rong > > > > by leaving PHY on reset state. > > >=20 > > > But it is. If we want to use ULPI as a bus like we do, then the PHY > > > will be no different then devices attached to many other buses. We > > > have made firmware fixes like that before. All the devices need to be > > > in a state, operational enough after bootup, so we can probe them in > > > OS without the need for hacks where they are separately enabled before > > > it's possible. > >=20 > > That makes no sense. Not only dwc3 and phy could live as modules (which > > means they may probe far away from device's boot time) but we have > > examples of buses not behaving like you said. E.g. I2C needs master to > > be probed to have bus working and no BIOS needs to make I2C controller > > functional in order to probe its controller's driver. >=20 > You can't really compare a bus like i2c, which can't enumerate devices > natively, to ULPI which can. 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 ? > > > > The real problem is what I stated above. > > > > With your current logic, you'll get stuck with checking/egg problem= : you > > > > need phy probed to probe dwc3, but need dwc3 probed to power on phy. > > > > You need a logic to break this circular dependency. > > >=20 > > > The moment we register the ulpi interface with the ulpi bus in > > > dwc3_probe(), we know dwc3 has it's PHY interface in operational mode > > > and register access to ULPI PHY is possible. And that is all dwc3 > > > needs to/can do. > > >=20 > > > I don't think you are seeing the whole "ulpi bus" in these patches, > > > but in any case; Like I said, this problem is purely BYT-CR specific, > > > which IMO really should be fixed in the firmware. > >=20 > > The proposed design has a flaw that breaks products on market simply > > because they don't have BIOS (unnecessarily) powering on phy. You're > > labeling that as BYT-CR specific issue because BYT-CR needs to be PM > > efficient and then it won't power on hw components in moment they don't > > need to. FW developers won't like this suggestion and I'd have to agree > > with them. >=20 > What exactly are we breaking here? The USB on BYT-CR does not work yet > with the mainline kernel, or does it? To enable it, I already > suggested the BYT quirk (attached again). one comment below on this. > I don't think the other boards we have which use TUSB1210, like the > BYT-I ones and I think some Merrifield based boards, expect any less > from PM efficiency then BYT-CR, but we don't need to do any tricks > with them in order to use ULPI bus. That is what I mean when I say > this is BYT-CR specific problem. perhaps because firmware on those other boards are powering up the PHY ? > 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. 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. > > > > > 2) Since the gpio resources are given to the controller device in= ACPI > > > > > tables and there isn't separate device object for the PHY at all,= we > > > > > need to deliver the gpios somehow separately to the phy driver. T= here > > > > > is a thread where we are talking about how to do that: > > > > > https://lkml.org/lkml/2014/12/18/82 > > > >=20 > > > > How about just leave the logic the way it is: > > > > dwc3-pci.c registers platform_device with gpio as resource if the G= PIOs > > > > are provided to dwc3. If not, then dwc3-pci.c will expect phy to ha= ve > > > > its own ACPI id. > > >=20 > > > I think you are now talking about the platform devices for the legacy > > > USB PHY framework created in dwc3-pci.c, which btw. were removed. > > > Please note that we are not using platform bus with the ULPI devices, > > > and those devices are created by the bus driver and not dwc3. > >=20 > > Yes, that the one. Current products' BIOS on market didn't know about n= ew > > ULPI bus. They rely on platform devices created by pci probe. Your ULPI > > bus proposal is way better to handle that problem and got my support > > since they beginning you showed that to me, but it does not justify > > breaking current working devices. Removing the platform device > > registration for phy with firmwares that rely on that was a mistake and > > any ACPI work related to fix that is unnecessary. These legacy ACPI > > tables gave the phy-related GPIOs to dwc3. Just mark is as legacy > > situation and let the legacy hw's happy. No vendor will change their > > BIOS after market due to non-buggy situation. >=20 > Well, I'm really not expecting any BIOS updates any more. I assumed > that was clear. Why else would I have started the whole planning of > the GPIO forwarding. But if it wasn't, then sorry. Now you know. >=20 > BYT-CR's USB is not supported in mainline yet unless I'm completely > mistaken, but we have the plan for it. Instead of trying to take any > shortcuts, let's follow that plan. >=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 I'll agree with this. > 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. it's not perfectly ok for dwc3 to toggle PHY's GPIOs. Have the PHY driver to that. > 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); > + } > + } why would you have dwc3 mess around with the PHY's gpios ? Doesn't look very good. --=20 balbi --WlEyl6ow+jlIgNUh Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUyl3HAAoJEIaOsuA1yqREYBQP/05qWgTWghKTOlQh7GohR+ax swYUzGqZbbAiQy5VStGOpZgt3G8ZqOW7us+AGyIJT0k0cZLg6Z1lmWSYM7n0Q03U zaSVfxcBTsXV15DjH0TMDgMW6oX7Bv6d5vYScudb5E5l9bSVJ9OrJJYYxw35PCWB 3oiRBDdUEb29+JZzBzoikyjGEaFqUSDG00Vhh4K+P3Q4hTIyiWjsZE8Y/DgzHw/j hJVplvJbbzg3OM64zOVZSKLujLb8qlyIIAkw2wSDqsaslxCJ7FIkeSIU2IgdJuX/ wEwyXF/ZBBKrKbMocfek+Tv9mXq2NvCV/j7/tqaCZRA7lBg5nYx7B5fnqo1lUnkm nmHJIu8Tx8U9iR+V9C2EtfU+PvaLgVZzzFSXd0fnUFFni2yi/puT4hYwInECuwb0 kt1ievubjasY+1J2GvzlqlYbmdQr2bCYeCNrxJDDKQDb8cLM5BKcWrLVd0r/dbiO iP8PIDd4pYgZTPnFzccJoWmpjppbiszb/PPRHjKMil4G9HgSTJ+bEjmRoyBTHBfP ItFHIffHIwJCkLazaS6FRW9bLGJ8hsO1v5kk1eWlFqBIHgympklUIRPZKKLfPluo wPeTA4b8UX2qScM8i9HYgpDOwnx6VGyudlD07CN/qL0ISM5+CpI0PGYnk2sYBw0h IdNCoEVZ/Evo6wh7cNL1 =mFB2 -----END PGP SIGNATURE----- --WlEyl6ow+jlIgNUh--