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

[David Cohen]

On Tue, Feb 10, 2015 at 11:05:31AM -0800, David Cohen wrote:
> 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.

Sorry, guess I spoke too soon :/
I am satisfied with the phy case, but I forgot about the chicken/egg
problem I reported earlier:
DWC3 will not be functional when reloading the module after it went to
reset state. Then ULPI enumeration can't happen regardless DSDT code
powered on phy.

Heikki, do you have a proposal for that? IMHO that's the main missing
point if we forget about BYT-CR legacy.

Br, David

> 
> > 
> > > > > > 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-usb" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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/