From: Felipe Balbi <balbi@ti.com>
To: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: David Cohen <david.a.cohen@linux.intel.com>,
Felipe Balbi <balbi@ti.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Baolu Lu <baolu.lu@linux.intel.com>, <linux-usb@vger.kernel.org>,
<linux-kernel@vger.kernel.org>,
Kishon Vijay Abraham I <kishon@ti.com>
Subject: Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
Date: Thu, 29 Jan 2015 10:20:23 -0600 [thread overview]
Message-ID: <20150129162023.GF21217@saruman.tx.rr.com> (raw)
In-Reply-To: <20150129141412.GA2570@kuha.fi.intel.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 --]
next prev parent reply other threads:[~2015-01-29 16:20 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-01-23 15:12 [PATCH 0/8] usb: ulpi bus Heikki Krogerus
2015-01-23 15:12 ` [PATCH 1/8] usb: add bus type for USB ULPI Heikki Krogerus
2015-01-29 5:02 ` Felipe Balbi
2015-01-29 14:18 ` Heikki Krogerus
2015-02-13 1:44 ` Stephen Boyd
2015-02-13 11:24 ` Heikki Krogerus
2015-01-23 15:12 ` [PATCH 2/8] usb: dwc3: USB2 PHY register access bits Heikki Krogerus
2015-01-23 15:12 ` [PATCH 3/8] usb: dwc3: store driver data earlier Heikki Krogerus
2015-01-23 15:12 ` [PATCH 4/8] usb: dwc3: cache hwparams earlier Heikki Krogerus
2015-01-23 15:12 ` [PATCH 5/8] usb: dwc3: ULPI or UTMI+ select Heikki Krogerus
2015-01-23 15:12 ` [PATCH 6/8] usb: dwc3: add ULPI interface support Heikki Krogerus
2015-01-23 16:24 ` Felipe Balbi
2015-01-26 11:46 ` Heikki Krogerus
2015-01-26 19:35 ` Felipe Balbi
2015-01-27 11:09 ` Heikki Krogerus
2015-01-27 15:24 ` Felipe Balbi
2015-02-11 19:34 ` David Cohen
2015-02-12 12:12 ` Heikki Krogerus
2015-02-13 1:41 ` David Cohen
2015-02-13 1:54 ` David Cohen
2015-02-13 13:16 ` Heikki Krogerus
2015-02-13 22:03 ` David Cohen
2015-02-13 22:04 ` Felipe Balbi
2015-01-23 15:12 ` [PATCH 7/8] phy: helpers for USB ULPI PHY registering Heikki Krogerus
2015-01-29 5:03 ` Felipe Balbi
2015-01-29 14:34 ` Heikki Krogerus
2015-01-29 16:11 ` Felipe Balbi
2015-01-30 10:33 ` Heikki Krogerus
2015-01-30 16:03 ` Felipe Balbi
2015-01-23 15:12 ` [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY Heikki Krogerus
2015-01-24 23:58 ` David Cohen
2015-01-26 12:55 ` Heikki Krogerus
2015-01-26 19:23 ` David Cohen
2015-01-27 9:28 ` Heikki Krogerus
2015-01-27 12:57 ` Heikki Krogerus
2015-01-27 17:38 ` David Cohen
2015-01-28 14:20 ` Heikki Krogerus
2015-01-28 18:02 ` David Cohen
2015-01-29 14:14 ` Heikki Krogerus
2015-01-29 16:20 ` Felipe Balbi [this message]
2015-01-29 18:02 ` David Cohen
2015-01-30 12:18 ` Heikki Krogerus
2015-01-30 16:09 ` David Cohen
2015-02-02 12:50 ` Heikki Krogerus
2015-01-30 9:29 ` Heikki Krogerus
2015-01-30 16:14 ` Felipe Balbi
2015-01-30 16:25 ` David Cohen
2015-01-30 16:30 ` Felipe Balbi
2015-01-30 16:20 ` David Cohen
2015-01-30 16:33 ` Felipe Balbi
2015-02-02 12:59 ` Heikki Krogerus
2015-02-03 11:37 ` Heikki Krogerus
2015-02-10 18:33 ` David Cohen
2015-02-10 19:05 ` David Cohen
2015-02-10 19:23 ` David Cohen
2015-02-11 13:12 ` Heikki Krogerus
2015-02-11 19:36 ` David Cohen
2015-02-13 22:02 ` David Cohen
2015-02-13 22:03 ` Felipe Balbi
2015-02-13 22:13 ` David Cohen
2015-01-29 5:09 ` Felipe Balbi
2015-01-29 14:30 ` Heikki Krogerus
2015-01-29 16:20 ` Felipe Balbi
2015-01-29 18:04 ` David Cohen
2015-01-29 18:25 ` David Cohen
2015-01-29 18:47 ` David Cohen
2015-01-30 10:30 ` Heikki Krogerus
2015-02-13 1:52 ` David Cohen
2015-02-13 12:35 ` Heikki Krogerus
2015-02-13 16:01 ` Felipe Balbi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150129162023.GF21217@saruman.tx.rr.com \
--to=balbi@ti.com \
--cc=baolu.lu@linux.intel.com \
--cc=david.a.cohen@linux.intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=heikki.krogerus@linux.intel.com \
--cc=kishon@ti.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).