On Fri, Jan 30, 2015 at 08:25:23AM -0800, David Cohen wrote: > On Fri, Jan 30, 2015 at 10:14:12AM -0600, Felipe Balbi wrote: > > 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. > > > > it doesn't make sense, what you say just doesn't make sense. You're > > assuming that a) only intel writes BIOS and b) you *always* have access > > to BIOS writers. You forget that companies other than Intel make x86 > > devices too. > > > > If the BIOS left the thing switched off, there's no "oh man, if only I > > had asked them to leave it on"... that's nonsense, just have the kernel > > deal with it. > > > > > > > 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 > > > > sure Heikki, no arguments there. But the fact of the matter is that the > > product David mentioned is *already* in the market. > > > > > 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. > > > > until then, we just have to deal with current state of affairs. > > > > > > > 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 > > > > > #include > > > > > #include > > > > > +#include > > > > > > > > > > #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. > > > > and what's the problem on doing this within PHY's probe ? The solution > > is simple: > > > > tusb1210_phy_probe() > > { > > ... > > > > gpiod_get(...); > > gpiod_direction_output(reset, 0); > > gpiod_set_value_cansleep(reset, 1); > > > > gpiod_get(...); > > gpiod_direction_output(cs, 0); > > gpiod_set_value_cansleep(cs, 1); > > > > eye = ulpi_read(); > > > > gpiod_set_value_cansleep(cs, 0); > > gpiod_put(cs); > > > > gpiod_set_value_cansleep(reset, 0); > > gpiod_put(reset); > > > > ... > > > > return 0; > > } > > > > This will have no effect on devices where PHY is already turned on and > > will cope with the device David mentioned. If, however, there's a way to > > get that eye diagram optimization without needing a ulpi_read() that's > > *even* better, otherwise, above should fine in all cases. > > A comment here. The ulpi_read() works for eye diagram only and only if > there is no reset on PHY between boot and probe. It is assuming the BIOS > set the correct value and then we just save it right away. As soon as we > reset the PHY this value is lost. So the GPIO toggling breaks the > current eye diagram request proposal. IMHO this should really come from > _DSD which will maintain the value regardless to what happens with phy. fair enough. -- balbi