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

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 8444 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]