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

[David Cohen <david.a.cohen@linux.intel.com>]

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.

That's a really wrong assumption. ULPI bus depends on dwc3 to be
functional and dwc3 depends on phy to be functional to complete its
power on sequence. We can't ask BIOS to let both up and running all the
time.

FWIW we *cannot* rely on ULPI bus enumeration to probe ULPI devices,
because it requires the ULPI device to be previously functional which
can't happen before the enumeration. Even if we ask BIOS to let phy
functional after boot, what happens when we remove modules and load it
again? Should we ask BIOS to power on the components again in order to
probe and power it on? It's a circular dependency you're creating.

> 
> > > 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 ?
> 
> Yes.

And that's wrong assumption. Not all fw will power on PHY. Maybe we
should stop all of other discussions and concentrate on this one:
BIOS will not guarantee phy will be functional after boot. And if we
remove modules and load again, it won't be functional regardless what
BIOS did during boot.

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

In order for phy to be functional, it does not depend only on toggling
GPIOs. It depends on DWC3 going to reset state, then phy executes power
on sequence, then DWC3 going out of reset state to sync clocks with phy.
You're saying we should tell BIOS is concurrently mess with dwc3
together with dwc3 driver?

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

Doesn't the ugly platform device case look less ugly than this?

Br, David

> 
> 
> Thanks,
> 
> -- 
> heikki