Message-ID: <20150130162523.GC20689@psi-dev26.jf.intel.com>
Date:	Fri, 30 Jan 2015 08:25:23 -0800
From:	David Cohen <david.a.cohen@...ux.intel.com>
To:	Felipe Balbi <balbi@...com>
Cc:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Baolu Lu <baolu.lu@...ux.intel.com>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Kishon Vijay Abraham I <kishon@...com>
Subject: Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY

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,
> > 
> > > > 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 ?
> > 
> > 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.
> > > 
> > > 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.
> > 
> > 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.
> > 
> > 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
> > > 
> > > 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.
> > 
> > Oh, I agree with 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 <linux/slab.h>
> > > >  #include <linux/pci.h>
> > > >  #include <linux/platform_device.h>
> > > > +#include <linux/gpio/consumer.h>
> > > >  
> > > >  #include "platform_data.h"
> > > >  
> > > > @@ -35,6 +36,24 @@
> > > >  
> > > >  static int dwc3_pci_quirks(struct pci_dev *pdev)
> > > >  {
> > > > +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > > > +	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > > > +		struct gpio_desc *gpio;
> > > > +
> > > > +		gpio = 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 = 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.
> > 
> > ..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 = 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.

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.

Br, David

> 
> cheers
> 
> -- 
> balbi


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists