Message-ID: <20150130161412.GD15318@saruman.tx.rr.com>
Date:	Fri, 30 Jan 2015 10:14:12 -0600
From:	Felipe Balbi <balbi@...com>
To:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
CC:	Felipe Balbi <balbi@...com>,
	David Cohen <david.a.cohen@...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 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.

cheers

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists