Hi On Thu, Jan 29, 2015 at 04:14:12PM +0200, Heikki Krogerus wrote: > > > > > > Can you share how tusb1210 is connected on the platform you're > > > > > > using as > > > > > > test for this patch? I don't think this driver would work reliably > > > > > > with > > > > > > this device: > > > > > > http://liliputing.com/2014/11/trekstor-launches-first-android-tablet-based-intels-irda-reference-design.html > > > > > > > > > > The only reason why that board doesn't work is because of very much > > > > > Baytrail-CR specific problems. These are are two issues, but the first > > > > > > > > That's not BYT-CR specific problems. That's just dwc3 and tusb1210 > > > > interacting as they're expecting to. > > > > > > > > > one is critical for getting it working. Both will be handled, but > > > > > separately from this set: > > > > > > > > > > 1) The firmware leaves the PHY in reset, forcing us to enable it > > > > > somehow in OS before accessing ulpi. Unless we can get a firmware fix > > > > > for that (it's starting to look like it's not going to happen; please > > > > > correct me if you know something else!), we need to add a quirk for > > > > > Baytrails (attached), which is probable still OK. But IMO this really > > > > > should be fixed in the firmware. > > > > > > > > It seems you're expecting the PHY to be fully operational in order to > > > > probe it. That's wrong assumption. BYT-CR's BIOS is doing nothing wrong > > > > by leaving PHY on reset state. > > > > > > But it is. If we want to use ULPI as a bus like we do, then the PHY > > > will be no different then devices attached to many other buses. We > > > have made firmware fixes like that before. All the devices need to be > > > in a state, operational enough after bootup, so we can probe them in > > > OS without the need for hacks where they are separately enabled before > > > it's possible. > > > > That makes no sense. Not only dwc3 and phy could live as modules (which > > means they may probe far away from device's boot time) but we have > > examples of buses not behaving like you said. E.g. I2C needs master to > > be probed to have bus working and no BIOS needs to make I2C controller > > functional in order to probe its controller's driver. > > 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 ? > > > > The real problem is what I stated above. > > > > With your current logic, you'll get stuck with checking/egg problem: you > > > > need phy probed to probe dwc3, but need dwc3 probed to power on phy. > > > > You need a logic to break this circular dependency. > > > > > > The moment we register the ulpi interface with the ulpi bus in > > > dwc3_probe(), we know dwc3 has it's PHY interface in operational mode > > > and register access to ULPI PHY is possible. And that is all dwc3 > > > needs to/can do. > > > > > > I don't think you are seeing the whole "ulpi bus" in these patches, > > > but in any case; Like I said, this problem is purely BYT-CR specific, > > > which IMO really should be fixed in the firmware. > > > > The proposed design has a flaw that breaks products on market simply > > because they don't have BIOS (unnecessarily) powering on phy. You're > > labeling that as BYT-CR specific issue because BYT-CR needs to be PM > > efficient and then it won't power on hw components in moment they don't > > need to. FW developers won't like this suggestion and I'd have to agree > > with them. > > What exactly are we breaking here? The USB on BYT-CR does not work yet > with the mainline kernel, or does it? To enable it, I already > suggested the BYT quirk (attached again). one comment below on this. > 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 ? > 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. > > > > > 2) Since the gpio resources are given to the controller device in ACPI > > > > > tables and there isn't separate device object for the PHY at all, we > > > > > need to deliver the gpios somehow separately to the phy driver. There > > > > > is a thread where we are talking about how to do that: > > > > > https://lkml.org/lkml/2014/12/18/82 > > > > > > > > How about just leave the logic the way it is: > > > > dwc3-pci.c registers platform_device with gpio as resource if the GPIOs > > > > are provided to dwc3. If not, then dwc3-pci.c will expect phy to have > > > > its own ACPI id. > > > > > > I think you are now talking about the platform devices for the legacy > > > USB PHY framework created in dwc3-pci.c, which btw. were removed. > > > Please note that we are not using platform bus with the ULPI devices, > > > and those devices are created by the bus driver and not dwc3. > > > > Yes, that the one. Current products' BIOS on market didn't know about new > > ULPI bus. They rely on platform devices created by pci probe. Your ULPI > > bus proposal is way better to handle that problem and got my support > > since they beginning you showed that to me, but it does not justify > > breaking current working devices. Removing the platform device > > registration for phy with firmwares that rely on that was a mistake and > > any ACPI work related to fix that is unnecessary. These legacy ACPI > > tables gave the phy-related GPIOs to dwc3. Just mark is as legacy > > situation and let the legacy hw's happy. No vendor will change their > > BIOS after market due to non-buggy situation. > > Well, I'm really not expecting any BIOS updates any more. I assumed > that was clear. Why else would I have started the whole planning of > the GPIO forwarding. But if it wasn't, then sorry. Now you know. > > BYT-CR's USB is not supported in mainline yet unless I'm completely > mistaken, but we have the plan for it. Instead of trying to take any > shortcuts, let's follow that plan. > > 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. > 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. -- balbi
signature.asc
Description: Digital signature
