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

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

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.

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

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

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


Thanks,

-- 
heikki