Message-ID: <20150129180223.GA2784@psi-dev26.jf.intel.com>
Date:	Thu, 29 Jan 2015 10:02:23 -0800
From:	David Cohen <david.a.cohen@...ux.intel.com>
To:	Felipe Balbi <balbi@...com>
Cc:	Heikki Krogerus <heikki.krogerus@...ux.intel.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

Hi Heikki, Felipe,

On Thu, Jan 29, 2015 at 10:20:23AM -0600, Felipe Balbi wrote:
> 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).

It used to work with mainline with minor restrictions. It stopped
working (and I failed to catch it during patch review) when NOP phy
enumeration was removed from dwc3-pci.c (but my understanding is that
Felipe is expecting to add it back as default phy in case no phy is
found by dwc3).

BYT-CR worked well except for operations that needed to access phy's
registers via ULPI bus (e.g. eye optimization). But to power on i.e.
TUSB1210 all you need it to toggle GPIOs, which is done by generic phy.
The need for ULPI bus was to complement this missing features, but
instead we're breaking BYT-CR :/

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

Let's propose this non-BYT-CR scenario:
You have dwc3/phy powered on by BIOS and dwc3 + phy as modules. You
probe the modules for the first time and it works because phy was
accessible. Then we remove the modules and load it again. By removing
the modules phy is not functional anymore (remove operation will put
them in reset state). Then you load the modules again. It certainly will
fail to access ULPI bus during phy's probe this time. BYT-CR is just an
example where BIOS lets phy in reset during probe, but you can get this
same behavior on other platforms too.

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

Agree.

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

It is supported, as I stated above. I don't know where you got the idea
it wasn't. I made BYT-T (and BYT-CR) + Merrifield USB device layer work
with mainline since 3.14. But this patch made the regression to stop
working:
https://www.mail-archive.com/linux-usb@vger.kernel.org/msg54400.html

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

I agree if we find a way to accomodate products already on market.

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

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.

I'd prefer to go back to platform device case (which is less ugly) for
products on market (i.e. legacy and unwanted way for future platforms).

Br, David

> 
> -- 
> balbi


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