Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY

[Felipe Balbi]

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,
> > > 
> > > > > 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.

fair enough.

-- 
balbi

Attachment: signature.asc
Description: Digital signature