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

[Heikki Krogerus]

> > > > 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 was wrong when I talked about BIOS powering the PHY before bootup. I
admit it. I'm saying this again as a reminder to myself: We can't mix
BIOS (or any FW) and ACPI. I mix them constantly. And I definitely
need to stop talking about bootup.

How this is handled is by letting the ACPI Power State methods of the
dwc3 device take care of the toggling of the gpios. It is done with
the help of something called GPIO OperationRegion. In case you are not
familiar with ACPI, then if you send me ACPI dump of one of those
devices (or just copy /sys/firmware/acpi/tables/DSDT), I can try to
modify the DSDT for you so you can use to test it, and provide you
with the ASL snippet.

You will see that the PHY is indeed in reset after bootup like it is
now (BIOS does nothing differently), but the gpios are automatically
toggled by the DSDT code. So every time you load dwc3 module, the PHY
will be operational when we need it, and when you unload dwc3, it will
be left in reset again. The OS does not have to do anything.

I really think that this BYT-CR case will be one of really rare
exception we will see, especially if we manage to put together the
ACPI table review that has been though about.

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

I don't understand what you are saying here?

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

The platform device would in any case need to be created only for this
case. We can't any more just create the phy device unconditionally for
every PCI platform like it was created before, as it's clear now the
PHY device can be be created from other sources. It was a risk always.

But the big problem is that since the PHY on your board is ULPI PHY,
it will make it really difficult to add the ULPI bus support. And like
I said in my previous mail, we would really need it for the boards
that expect the PHY driver to provide functions like charger
detection. We don't need it only for BYT based boards.

So on top of the above, since the GPIO resources are given to dwc3, I
actually think that my hack is better then the platform device.


Thanks,

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