Message-ID: <20150210190531.GC28827@psi-dev26.jf.intel.com>
Date:	Tue, 10 Feb 2015 11:05:31 -0800
From:	David Cohen <david.a.cohen@...ux.intel.com>
To:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:	Felipe Balbi <balbi@...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 Mon, Feb 02, 2015 at 02:59:59PM +0200, Heikki Krogerus wrote:
> > > > > 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.

Hm. That changes everything :)
It's a feasible direction to push vendors.

> 
> 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 hope it will.

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

TUSB1210 needs to come out of reset only when DWC3 is in reset state.
This is how current code works in dwc3_core_soft_reset():
- dwc3 goes to reset
- phy goes to reset
- phy gets out of reset
- dwc3 gets out of reset

This is how you're proposing:
- phy goes to reset (DSDT code, when loading module)
- phy gets out of reset (DSDT code, when loading module)

- dwc3 goes to reset (dwc3_core_soft_reset())
- dwc3 gets our of reset (dwc3_core_soft_reset())

Felipe, do you see a problem with this new context? If not, I'm
satisfied with Heikki's ULPI bus proposal considering my comment below.

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

This would bring support to current BYT-CR products, but it's a lot ugly :/
But current DSDT is a lot ugly too.

Felipe, we do need ULPI bus working for BYT-CR due to 2 scenarios:
- we need eye optimization
- we need to pull down D+/- on phy after cable disconnection so charging
  detection module can detect CDP connection during next cable plug in.

I see no way to make current BYT-CR platforms work with Heikki's ULPI
bus proposal without crippling it if we don't toggle these GPIOs outside
ULPI's device probe.

>From my side, the quirk is acceptable as a trade off to not damage his
design due to legacy platforms.

Br, David

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

Powered by blists - more mailing lists