Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
Hi Heikki,
On Fri, Jan 23, 2015 at 05:12:58PM +0200, Heikki Krogerus wrote:
> TUSB1210 ULPI PHY has vendor specific register for eye
> diagram tuning. On some platforms the system firmware has
> set optimized value to it. In order to not loose the
> optimized value, the driver stores it during probe and
> restores it every time the PHY is powered back on.
>
> Signed-off-by: Heikki Krogerus
> Cc: Kishon Vijay Abraham I
> ---
> drivers/phy/Kconfig| 6 ++
> drivers/phy/Makefile | 1 +
> drivers/phy/phy-tusb1210.c | 133
> +
> 3 files changed, 140 insertions(+)
> create mode 100644 drivers/phy/phy-tusb1210.c
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 26a7623..52ee720 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -284,4 +284,10 @@ config PHY_QCOM_UFS
> help
> Support for UFS PHY on QCOM chipsets.
>
> +config PHY_TUSB1210
> + tristate "TI TUSB1210 ULPI PHY module"
> + depends on USB_ULPI_BUS
> + help
> + Support for TI TUSB1210 USB ULPI PHY.
> +
> endmenu
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index cfbb720..03f3d85 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -37,3 +37,4 @@ obj-$(CONFIG_PHY_STIH41X_USB) +=
> phy-stih41x-usb.o
> obj-$(CONFIG_PHY_QCOM_UFS) += phy-qcom-ufs.o
> obj-$(CONFIG_PHY_QCOM_UFS) += phy-qcom-ufs-qmp-20nm.o
> obj-$(CONFIG_PHY_QCOM_UFS) += phy-qcom-ufs-qmp-14nm.o
> +obj-$(CONFIG_PHY_TUSB1210) += phy-tusb1210.o
> diff --git a/drivers/phy/phy-tusb1210.c b/drivers/phy/phy-tusb1210.c
> new file mode 100644
> index 000..1551ae8
> --- /dev/null
> +++ b/drivers/phy/phy-tusb1210.c
> @@ -0,0 +1,133 @@
> +/**
> + * tusb1210.c - TUSB1210 USB ULPI PHY driver
> + *
> + * Copyright (C) 2015 Intel Corporation
> + *
> + * Author: Heikki Krogerus
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include
> +#include
> +#include
> +
> +#include "ulpi_phy.h"
> +
> +#define TUSB1210_VENDOR_SPECIFIC2 0x80
> +
> +struct tusb1210 {
> + struct ulpi *ulpi;
> + struct phy *phy;
> + struct gpio_desc *gpio_reset;
> + struct gpio_desc *gpio_cs;
> + u8 eye_diagram_tuning;
> +};
> +
> +static int tusb1210_power_on(struct phy *phy)
> +{
> + struct tusb1210 *tusb = phy_get_drvdata(phy);
> +
> + gpiod_set_value_cansleep(tusb->gpio_reset, 1);
> + gpiod_set_value_cansleep(tusb->gpio_cs, 1);
> +
> + /* Restore eye diagram optimisation value */
> + ulpi_write(tusb->ulpi, TUSB1210_VENDOR_SPECIFIC2,
> +tusb->eye_diagram_tuning);
After you power on phy, ulpi bus may not be available right away. In
intel case, phy power on happens during dwc3 power on. ULPI bus is not
available until OTG controller and phy are in sync.
In resume, you can't restore eye diagram from here.
> +
> + return 0;
> +}
> +
> +static int tusb1210_power_off(struct phy *phy)
> +{
> + struct tusb1210 *tusb = phy_get_drvdata(phy);
> +
> + gpiod_set_value_cansleep(tusb->gpio_reset, 0);
> + gpiod_set_value_cansleep(tusb->gpio_cs, 0);
> +
> + return 0;
> +}
> +
> +static struct phy_ops phy_ops = {
> + .power_on = tusb1210_power_on,
> + .power_off = tusb1210_power_off,
> + .init = tusb1210_power_on,
> + .exit = tusb1210_power_off,
> + .owner = THIS_MODULE,
> +};
> +
> +static int tusb1210_probe(struct ulpi *ulpi)
> +{
> + struct gpio_desc *gpio;
> + struct tusb1210 *tusb;
> + int ret;
> +
> + tusb = devm_kzalloc(&ulpi->dev, sizeof(*tusb), GFP_KERNEL);
> + if (!tusb)
> + return -ENOMEM;
> +
> + gpio = devm_gpiod_get(&ulpi->dev, "reset");
> + if (!IS_ERR(gpio)) {
> + ret = gpiod_direction_output(gpio, 0);
> + if (ret)
> + return ret;
> + tusb->gpio_reset = gpio;
> + }
You cannot proceed with probe if gpio reset is not available. Different
from CS, it's a mandatory pin to toggle in order to power on/off phy and
get it in sync with OTG controller.
> +
> + gpio = devm_gpiod_get(&ulpi->dev, "cs");
> + if (!IS_ERR(gpio)) {
> + ret = gpiod_direction_output(gpio, 0);
> + if (ret)
> + return ret;
> + tusb->gpio_cs = gpio;
> + }
> +
> + /* Store initial eye diagram optimisation value */
> + ret = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
It's unclear if ulpi is accessible at this point. You can't read it at
this point.
Br, David
> + if (ret < 0)
> + return ret;
> +
> + tusb->eye_diagram_tuning = ret;
> +
> + tusb->phy = ulpi_phy_create(ulpi, &phy_ops);
> + if (IS_ERR(tusb->phy))
> + return PTR_ERR(tusb->phy);
> +
> + tusb->ulpi = ulpi;
> +
> + phy_set_drvdata(tu
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
Hi David,
On Sat, Jan 24, 2015 at 03:58:11PM -0800, David Cohen wrote:
> > +static int tusb1210_power_on(struct phy *phy)
> > +{
> > + struct tusb1210 *tusb = phy_get_drvdata(phy);
> > +
> > + gpiod_set_value_cansleep(tusb->gpio_reset, 1);
> > + gpiod_set_value_cansleep(tusb->gpio_cs, 1);
> > +
> > + /* Restore eye diagram optimisation value */
> > + ulpi_write(tusb->ulpi, TUSB1210_VENDOR_SPECIFIC2,
> > + tusb->eye_diagram_tuning);
>
> After you power on phy, ulpi bus may not be available right away. In
> intel case, phy power on happens during dwc3 power on. ULPI bus is not
> available until OTG controller and phy are in sync.
>
> In resume, you can't restore eye diagram from here.
I'm sorry but I don't think I understand? Where do we power on the phy
before dwc3 is powered on? Or is this a Baytrail-CR specific problem?
I can't see any problems with the hardware I have.
In any case, this sounds like purely dwc3 issue and not tusb1210
issue.
> > +static int tusb1210_probe(struct ulpi *ulpi)
> > +{
> > + struct gpio_desc *gpio;
> > + struct tusb1210 *tusb;
> > + int ret;
> > +
> > + tusb = devm_kzalloc(&ulpi->dev, sizeof(*tusb), GFP_KERNEL);
> > + if (!tusb)
> > + return -ENOMEM;
> > +
> > + gpio = devm_gpiod_get(&ulpi->dev, "reset");
> > + if (!IS_ERR(gpio)) {
> > + ret = gpiod_direction_output(gpio, 0);
> > + if (ret)
> > + return ret;
> > + tusb->gpio_reset = gpio;
> > + }
>
> You cannot proceed with probe if gpio reset is not available. Different
> from CS, it's a mandatory pin to toggle in order to power on/off phy and
> get it in sync with OTG controller.
Well, let's check -ENOENT and -ENODEV return values separately. The
reset pin is not used on all platforms so getting this gpio is
optional. This is the case even with some Intel's platforms using
TUSB1210.
> > +
> > + gpio = devm_gpiod_get(&ulpi->dev, "cs");
> > + if (!IS_ERR(gpio)) {
> > + ret = gpiod_direction_output(gpio, 0);
> > + if (ret)
> > + return ret;
> > + tusb->gpio_cs = gpio;
> > + }
> > +
> > + /* Store initial eye diagram optimisation value */
> > + ret = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
>
> It's unclear if ulpi is accessible at this point. You can't read it at
> this point.
We wouldn't have reached this point if ulpi wasn't accessible.
Registering the ulpi interface would have already failed so no driver
would have been probed.
Thanks!
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
Hi Heikki,
On Mon, Jan 26, 2015 at 02:55:03PM +0200, Heikki Krogerus wrote:
> Hi David,
>
> On Sat, Jan 24, 2015 at 03:58:11PM -0800, David Cohen wrote:
> > > +static int tusb1210_power_on(struct phy *phy)
> > > +{
> > > + struct tusb1210 *tusb = phy_get_drvdata(phy);
> > > +
> > > + gpiod_set_value_cansleep(tusb->gpio_reset, 1);
> > > + gpiod_set_value_cansleep(tusb->gpio_cs, 1);
> > > +
> > > + /* Restore eye diagram optimisation value */
> > > + ulpi_write(tusb->ulpi, TUSB1210_VENDOR_SPECIFIC2,
> > > +tusb->eye_diagram_tuning);
> >
> > After you power on phy, ulpi bus may not be available right away. In
> > intel case, phy power on happens during dwc3 power on. ULPI bus is not
> > available until OTG controller and phy are in sync.
> >
> > In resume, you can't restore eye diagram from here.
>
> I'm sorry but I don't think I understand? Where do we power on the phy
> before dwc3 is powered on? Or is this a Baytrail-CR specific problem?
> I can't see any problems with the hardware I have.
You can't see in single accesses. But you may see when running stability
tests overnight.
Anyway, look for dwc3_core_soft_reset() function:
- dwc3 goes to reset state
- phy is initialized (or at least gets ready to sync clocks)
- dwc3 goes out or reset state
During tusb1210 phy init from dwc3, you shouldn't access ulpi bus.
>
> In any case, this sounds like purely dwc3 issue and not tusb1210
> issue.
That's neither purely dwc3's not tusb1210's, that's your problem :)
Since it's a potential bug introduced by your patch set here.
>
> > > +static int tusb1210_probe(struct ulpi *ulpi)
> > > +{
> > > + struct gpio_desc *gpio;
> > > + struct tusb1210 *tusb;
> > > + int ret;
> > > +
> > > + tusb = devm_kzalloc(&ulpi->dev, sizeof(*tusb), GFP_KERNEL);
> > > + if (!tusb)
> > > + return -ENOMEM;
> > > +
> > > + gpio = devm_gpiod_get(&ulpi->dev, "reset");
> > > + if (!IS_ERR(gpio)) {
> > > + ret = gpiod_direction_output(gpio, 0);
> > > + if (ret)
> > > + return ret;
> > > + tusb->gpio_reset = gpio;
> > > + }
> >
> > You cannot proceed with probe if gpio reset is not available. Different
> > from CS, it's a mandatory pin to toggle in order to power on/off phy and
> > get it in sync with OTG controller.
>
> Well, let's check -ENOENT and -ENODEV return values separately. The
> reset pin is not used on all platforms so getting this gpio is
> optional. This is the case even with some Intel's platforms using
> TUSB1210.
I doublechecked the tusb1210 datasheet. Despite the power on sequence
mentions RESET toggling as required, it has a comment later on that it
can be tied to VDDIO.
So as you mentioned, it'd be better to ignore -ENOENT and -ENODEV and
raise error otherwise.
>
> > > +
> > > + gpio = devm_gpiod_get(&ulpi->dev, "cs");
> > > + if (!IS_ERR(gpio)) {
> > > + ret = gpiod_direction_output(gpio, 0);
> > > + if (ret)
> > > + return ret;
> > > + tusb->gpio_cs = gpio;
> > > + }
> > > +
> > > + /* Store initial eye diagram optimisation value */
> > > + ret = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
> >
> > It's unclear if ulpi is accessible at this point. You can't read it at
> > this point.
>
> We wouldn't have reached this point if ulpi wasn't accessible.
> Registering the ulpi interface would have already failed so no driver
> would have been probed.
You have a chicken/egg problem here:
- dwc3 needs phy to complete soft reset during probe
- tusb1210 needs dwc3 soft reset completed to be accessible via ULPI
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
Br, David
>
>
> Thanks!
>
> --
> heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Mon, Jan 26, 2015 at 11:23:37AM -0800, David Cohen wrote:
> On Mon, Jan 26, 2015 at 02:55:03PM +0200, Heikki Krogerus wrote:
> > On Sat, Jan 24, 2015 at 03:58:11PM -0800, David Cohen wrote:
> > > > +static int tusb1210_power_on(struct phy *phy)
> > > > +{
> > > > + struct tusb1210 *tusb = phy_get_drvdata(phy);
> > > > +
> > > > + gpiod_set_value_cansleep(tusb->gpio_reset, 1);
> > > > + gpiod_set_value_cansleep(tusb->gpio_cs, 1);
> > > > +
> > > > + /* Restore eye diagram optimisation value */
> > > > + ulpi_write(tusb->ulpi, TUSB1210_VENDOR_SPECIFIC2,
> > > > + tusb->eye_diagram_tuning);
> > >
> > > After you power on phy, ulpi bus may not be available right away. In
> > > intel case, phy power on happens during dwc3 power on. ULPI bus is not
> > > available until OTG controller and phy are in sync.
> > >
> > > In resume, you can't restore eye diagram from here.
> >
> > I'm sorry but I don't think I understand? Where do we power on the phy
> > before dwc3 is powered on? Or is this a Baytrail-CR specific problem?
> > I can't see any problems with the hardware I have.
>
> You can't see in single accesses. But you may see when running stability
> tests overnight.
>
> Anyway, look for dwc3_core_soft_reset() function:
> - dwc3 goes to reset state
> - phy is initialized (or at least gets ready to sync clocks)
> - dwc3 goes out or reset state
And if you look at dwc3_probe, you'll notice that it calls
phy_power_on after that, and ulpi will most definitely be accessible
at that point.
I'm only using the init and exit hooks instead of just
power_on/power_off because I'm not sure which the controllers will
use. For example, now dwc3 calls phy_init() in it's resume routine and
not phy_power_on() like I would expect. I know the problem has been
pointed out by others, so I'm expecting we'll get guidelines for it
later. But before we do, I see no harm in having both init and
power_on hooks in this driver.
> During tusb1210 phy init from dwc3, you shouldn't access ulpi bus.
We will end up executing one failed write followed by write that we
know will succeed. Ideally we didn't have to do the first one at all,
but I don't see any risk here.
> > > > + gpio = devm_gpiod_get(&ulpi->dev, "cs");
> > > > + if (!IS_ERR(gpio)) {
> > > > + ret = gpiod_direction_output(gpio, 0);
> > > > + if (ret)
> > > > + return ret;
> > > > + tusb->gpio_cs = gpio;
> > > > + }
> > > > +
> > > > + /* Store initial eye diagram optimisation value */
> > > > + ret = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
> > >
> > > It's unclear if ulpi is accessible at this point. You can't read it at
> > > this point.
> >
> > We wouldn't have reached this point if ulpi wasn't accessible.
> > Registering the ulpi interface would have already failed so no driver
> > would have been probed.
>
> You have a chicken/egg problem here:
> - dwc3 needs phy to complete soft reset during probe
> - tusb1210 needs dwc3 soft reset completed to be accessible via ULPI
>
> 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
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.
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
Thanks,
--
heikki
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
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
Hi guys, > I'm only using the init and exit hooks instead of just > power_on/power_off because I'm not sure which the controllers will > use. For example, now dwc3 calls phy_init() in it's resume routine and > not phy_power_on() like I would expect. I know the problem has been > pointed out by others, so I'm expecting we'll get guidelines for it > later. But before we do, I see no harm in having both init and > power_on hooks in this driver. I'm unable to find the thread where somebody mentioned that, but I don't actually think there is any reason why we couldn't just call phy_power_on/off instead of phy_init/exit in dwc3_suspend/resume like I think we should. I'll add one more patch where I'll change it. Felipe, is it OK? Thanks, -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Tue, Jan 27, 2015 at 11:28:56AM +0200, Heikki Krogerus wrote:
> On Mon, Jan 26, 2015 at 11:23:37AM -0800, David Cohen wrote:
> > On Mon, Jan 26, 2015 at 02:55:03PM +0200, Heikki Krogerus wrote:
> > > On Sat, Jan 24, 2015 at 03:58:11PM -0800, David Cohen wrote:
> > > > > +static int tusb1210_power_on(struct phy *phy)
> > > > > +{
> > > > > + struct tusb1210 *tusb = phy_get_drvdata(phy);
> > > > > +
> > > > > + gpiod_set_value_cansleep(tusb->gpio_reset, 1);
> > > > > + gpiod_set_value_cansleep(tusb->gpio_cs, 1);
> > > > > +
> > > > > + /* Restore eye diagram optimisation value */
> > > > > + ulpi_write(tusb->ulpi, TUSB1210_VENDOR_SPECIFIC2,
> > > > > +tusb->eye_diagram_tuning);
> > > >
> > > > After you power on phy, ulpi bus may not be available right away. In
> > > > intel case, phy power on happens during dwc3 power on. ULPI bus is not
> > > > available until OTG controller and phy are in sync.
> > > >
> > > > In resume, you can't restore eye diagram from here.
> > >
> > > I'm sorry but I don't think I understand? Where do we power on the phy
> > > before dwc3 is powered on? Or is this a Baytrail-CR specific problem?
> > > I can't see any problems with the hardware I have.
> >
> > You can't see in single accesses. But you may see when running stability
> > tests overnight.
> >
> > Anyway, look for dwc3_core_soft_reset() function:
> > - dwc3 goes to reset state
> > - phy is initialized (or at least gets ready to sync clocks)
> > - dwc3 goes out or reset state
>
> And if you look at dwc3_probe, you'll notice that it calls
> phy_power_on after that, and ulpi will most definitely be accessible
> at that point.
>
> I'm only using the init and exit hooks instead of just
> power_on/power_off because I'm not sure which the controllers will
> use. For example, now dwc3 calls phy_init() in it's resume routine and
> not phy_power_on() like I would expect. I know the problem has been
> pointed out by others, so I'm expecting we'll get guidelines for it
> later. But before we do, I see no harm in having both init and
> power_on hooks in this driver.
It sounds hackish to me. You could clearly change the logic on
init/power_on callbacks to avoid this problem.
>
> > During tusb1210 phy init from dwc3, you shouldn't access ulpi bus.
>
> We will end up executing one failed write followed by write that we
> know will succeed. Ideally we didn't have to do the first one at all,
> but I don't see any risk here.
Ditto.
>
> > > > > + gpio = devm_gpiod_get(&ulpi->dev, "cs");
> > > > > + if (!IS_ERR(gpio)) {
> > > > > + ret = gpiod_direction_output(gpio, 0);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > + tusb->gpio_cs = gpio;
> > > > > + }
> > > > > +
> > > > > + /* Store initial eye diagram optimisation value */
> > > > > + ret = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
> > > >
> > > > It's unclear if ulpi is accessible at this point. You can't read it at
> > > > this point.
> > >
> > > We wouldn't have reached this point if ulpi wasn't accessible.
> > > Registering the ulpi interface would have already failed so no driver
> > > would have been probed.
> >
> > You have a chicken/egg problem here:
> > - dwc3 needs phy to complete soft reset during probe
> > - tusb1210 needs dwc3 soft reset completed to be accessible via ULPI
> >
> > 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. 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.
>
> 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 d
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
Hi, > > > 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. > 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. > > 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. I'm pretty sure now that you are not seeing the whole point with this set, which is to provide new bus type for ULPI. If so, then could you please review the whole series. Thanks, -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Wed, Jan 28, 2015 at 04:20:25PM +0200, Heikki Krogerus wrote: > Hi, > > > > > 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. > > > 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. > > > > 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. > > I'm pretty sure now that you are not seeing the whole point with this > set, which is to provide new bus type for ULPI. If so, then could you > please review the whole series. I'll review the whole series. But pls note your design requires vendors to change their FW (including but not limited to ACPI table) after market. That won't happen. Br, David > > > Than
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Fri, Jan 23, 2015 at 05:12:58PM +0200, Heikki Krogerus wrote:
> TUSB1210 ULPI PHY has vendor specific register for eye
> diagram tuning. On some platforms the system firmware has
> set optimized value to it. In order to not loose the
> optimized value, the driver stores it during probe and
> restores it every time the PHY is powered back on.
>
> Signed-off-by: Heikki Krogerus
> Cc: Kishon Vijay Abraham I
> ---
> drivers/phy/Kconfig| 6 ++
> drivers/phy/Makefile | 1 +
> drivers/phy/phy-tusb1210.c | 133
> +
> 3 files changed, 140 insertions(+)
> create mode 100644 drivers/phy/phy-tusb1210.c
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 26a7623..52ee720 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -284,4 +284,10 @@ config PHY_QCOM_UFS
> help
> Support for UFS PHY on QCOM chipsets.
>
> +config PHY_TUSB1210
> + tristate "TI TUSB1210 ULPI PHY module"
> + depends on USB_ULPI_BUS
> + help
> + Support for TI TUSB1210 USB ULPI PHY.
> +
> endmenu
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index cfbb720..03f3d85 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -37,3 +37,4 @@ obj-$(CONFIG_PHY_STIH41X_USB) +=
> phy-stih41x-usb.o
> obj-$(CONFIG_PHY_QCOM_UFS) += phy-qcom-ufs.o
> obj-$(CONFIG_PHY_QCOM_UFS) += phy-qcom-ufs-qmp-20nm.o
> obj-$(CONFIG_PHY_QCOM_UFS) += phy-qcom-ufs-qmp-14nm.o
> +obj-$(CONFIG_PHY_TUSB1210) += phy-tusb1210.o
> diff --git a/drivers/phy/phy-tusb1210.c b/drivers/phy/phy-tusb1210.c
> new file mode 100644
> index 000..1551ae8
> --- /dev/null
> +++ b/drivers/phy/phy-tusb1210.c
> @@ -0,0 +1,133 @@
> +/**
> + * tusb1210.c - TUSB1210 USB ULPI PHY driver
> + *
> + * Copyright (C) 2015 Intel Corporation
> + *
> + * Author: Heikki Krogerus
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include
> +#include
> +#include
> +
> +#include "ulpi_phy.h"
> +
> +#define TUSB1210_VENDOR_SPECIFIC2 0x80
> +
> +struct tusb1210 {
> + struct ulpi *ulpi;
> + struct phy *phy;
> + struct gpio_desc *gpio_reset;
> + struct gpio_desc *gpio_cs;
> + u8 eye_diagram_tuning;
> +};
> +
> +static int tusb1210_power_on(struct phy *phy)
> +{
> + struct tusb1210 *tusb = phy_get_drvdata(phy);
> +
> + gpiod_set_value_cansleep(tusb->gpio_reset, 1);
> + gpiod_set_value_cansleep(tusb->gpio_cs, 1);
> +
> + /* Restore eye diagram optimisation value */
> + ulpi_write(tusb->ulpi, TUSB1210_VENDOR_SPECIFIC2,
> +tusb->eye_diagram_tuning);
> +
> + return 0;
> +}
> +
> +static int tusb1210_power_off(struct phy *phy)
> +{
> + struct tusb1210 *tusb = phy_get_drvdata(phy);
> +
> + gpiod_set_value_cansleep(tusb->gpio_reset, 0);
> + gpiod_set_value_cansleep(tusb->gpio_cs, 0);
> +
> + return 0;
> +}
> +
> +static struct phy_ops phy_ops = {
> + .power_on = tusb1210_power_on,
> + .power_off = tusb1210_power_off,
> + .init = tusb1210_power_on,
> + .exit = tusb1210_power_off,
these should not be the same. It looks like what you want is for reset
and cs gpios to be handle on init/exit and ulpi_write() from power_on().
Also, I was chatting in private with David and, apparently, there's a
way to request for eye diagram data from BIOS straight. That's more
in-line with what we would do for DT-based boots, passing that
eye-diagram data as a DT attribute.
Care to comment ? If that's the case, I'd rather use that BIOS hook and
remove ulpi_read() from probe().
> + .owner = THIS_MODULE,
> +};
> +
> +static int tusb1210_probe(struct ulpi *ulpi)
> +{
> + struct gpio_desc *gpio;
> + struct tusb1210 *tusb;
> + int ret;
> +
> + tusb = devm_kzalloc(&ulpi->dev, sizeof(*tusb), GFP_KERNEL);
> + if (!tusb)
> + return -ENOMEM;
> +
> + gpio = devm_gpiod_get(&ulpi->dev, "reset");
> + if (!IS_ERR(gpio)) {
> + ret = gpiod_direction_output(gpio, 0);
> + if (ret)
> + return ret;
> + tusb->gpio_reset = gpio;
> + }
> +
> + gpio = devm_gpiod_get(&ulpi->dev, "cs");
> + if (!IS_ERR(gpio)) {
> + ret = gpiod_direction_output(gpio, 0);
> + if (ret)
> + return ret;
> + tusb->gpio_cs = gpio;
> + }
> +
right before this call, you really don't know the state of either CS nor
RESET gpios, so it could very well be that firmware left it disabled.
There are cases where that extra uW of runtime power consumption
matters, you probably still remember the weeks we spent optimizing
regulator usage on both twl4030-usb and isp17xx (whatever that was) on
N900.
Either wrap this call with proper gpio handling (if there's really no
other way to get the eye
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
> > > > > 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. > > > 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). 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. 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. > > > > 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
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
> Also, I was chatting in private with David and, apparently, there's a > way to request for eye diagram data from BIOS straight. That's more > in-line with what we would do for DT-based boots, passing that > eye-diagram data as a DT attribute. > > Care to comment ? If that's the case, I'd rather use that BIOS hook and > remove ulpi_read() from probe(). I'm not familiar with such method. But if there is one, I'm not against it. I need to check it. Thanks, -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Thu, Jan 29, 2015 at 04:30:53PM +0200, Heikki Krogerus wrote: > > Also, I was chatting in private with David and, apparently, there's a > > way to request for eye diagram data from BIOS straight. That's more > > in-line with what we would do for DT-based boots, passing that > > eye-diagram data as a DT attribute. > > > > Care to comment ? If that's the case, I'd rather use that BIOS hook and > > remove ulpi_read() from probe(). > > I'm not familiar with such method. But if there is one, I'm not > against it. I need to check it. David ? Care to comment ? -- balbi signature.asc Description: Digital signature
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
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 deli
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
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Thu, Jan 29, 2015 at 10:20:49AM -0600, Felipe Balbi wrote: > On Thu, Jan 29, 2015 at 04:30:53PM +0200, Heikki Krogerus wrote: > > > Also, I was chatting in private with David and, apparently, there's a > > > way to request for eye diagram data from BIOS straight. That's more > > > in-line with what we would do for DT-based boots, passing that > > > eye-diagram data as a DT attribute. > > > > > > Care to comment ? If that's the case, I'd rather use that BIOS hook and > > > remove ulpi_read() from probe(). > > > > I'm not familiar with such method. But if there is one, I'm not > > against it. I need to check it. > > David ? Care to comment ? I agree with Heikki's proposal to use BIOS hook and remove ulpi_read() from probe(). That address one of my chicken/egg concerns. Br, David > > -- > balbi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Thu, Jan 29, 2015 at 10:04:16AM -0800, David Cohen wrote: > On Thu, Jan 29, 2015 at 10:20:49AM -0600, Felipe Balbi wrote: > > On Thu, Jan 29, 2015 at 04:30:53PM +0200, Heikki Krogerus wrote: > > > > Also, I was chatting in private with David and, apparently, there's a > > > > way to request for eye diagram data from BIOS straight. That's more > > > > in-line with what we would do for DT-based boots, passing that > > > > eye-diagram data as a DT attribute. > > > > > > > > Care to comment ? If that's the case, I'd rather use that BIOS hook and > > > > remove ulpi_read() from probe(). > > > > > > I'm not familiar with such method. But if there is one, I'm not > > > against it. I need to check it. > > > > David ? Care to comment ? > > I agree with Heikki's proposal to use BIOS hook and remove ulpi_read() > from probe(). That address one of my chicken/egg concerns. Oops. I misinterpreted the thread. Heikki didn't propose that :) Let me get things mature and reply to here. Br, David > > Br, David > > > > > -- > > balbi > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Thu, Jan 29, 2015 at 10:25:38AM -0800, David Cohen wrote: > On Thu, Jan 29, 2015 at 10:04:16AM -0800, David Cohen wrote: > > On Thu, Jan 29, 2015 at 10:20:49AM -0600, Felipe Balbi wrote: > > > On Thu, Jan 29, 2015 at 04:30:53PM +0200, Heikki Krogerus wrote: > > > > > Also, I was chatting in private with David and, apparently, there's a > > > > > way to request for eye diagram data from BIOS straight. That's more > > > > > in-line with what we would do for DT-based boots, passing that > > > > > eye-diagram data as a DT attribute. > > > > > > > > > > Care to comment ? If that's the case, I'd rather use that BIOS hook > > > > > and > > > > > remove ulpi_read() from probe(). > > > > > > > > I'm not familiar with such method. But if there is one, I'm not > > > > against it. I need to check it. > > > > > > David ? Care to comment ? > > > > I agree with Heikki's proposal to use BIOS hook and remove ulpi_read() > > from probe(). That address one of my chicken/egg concerns. > > Oops. I misinterpreted the thread. Heikki didn't propose that :) > Let me get things mature and reply to here. I talked to David Box (CC'ed him to this reply). We can add an integer value to ACPI and request it during phy's probe using ACPI's Device Specific Data (_DSD). That would end the need to have phy functional during probe (being compatible to BYT-CR and to module support) and would be more compatible to DT as well. Br, David > > Br, David > > > > > Br, David > > > > > > > > -- > > > balbi > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
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.
> > 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 ?
Yes.
> > 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
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.
> > 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.
Thanks,
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
> > > > > > Also, I was chatting in private with David and, apparently, there's > > > > > > a > > > > > > way to request for eye diagram data from BIOS straight. That's more > > > > > > in-line with what we would do for DT-based boots, passing that > > > > > > eye-diagram data as a DT attribute. > > > > > > > > > > > > Care to comment ? If that's the case, I'd rather use that BIOS hook > > > > > > and > > > > > > remove ulpi_read() from probe(). > > > > > > > > > > I'm not familiar with such method. But if there is one, I'm not > > > > > against it. I need to check it. > > > > > > > > David ? Care to comment ? > > > > > > I agree with Heikki's proposal to use BIOS hook and remove ulpi_read() > > > from probe(). That address one of my chicken/egg concerns. > > > > Oops. I misinterpreted the thread. Heikki didn't propose that :) > > Let me get things mature and reply to here. > > I talked to David Box (CC'ed him to this reply). > We can add an integer value to ACPI and request it during phy's probe > using ACPI's Device Specific Data (_DSD). That would end the need to > have phy functional during probe (being compatible to BYT-CR and to > module support) and would be more compatible to DT as well. You can still make modifications to the DSDT?! I am familiar with _DSD, and it does not just allow us to add integer values, but complete named key to value properties compatible with dt, including strings. And in case you guys were not familiar with the unified device property interface, it's a wrapper for apci and dt, and should btw. be used instead of acpi_* or of_* to get properties in drivers from now on. So if we add these properties for the dwc3 device object: "ulpi,vendor", "ulpi,product", we can check them in ULPI bus driver as an alternative for reading the vendor and product registers, and indeed be able to bind the PHY to a driver before it's powered on! P.S. We should not mix BIOS to ACPI. Thanks! -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
Hi,
> > > 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 :/
So what you are saying that if I get one of those devices you
mentioned and try vanilla kernel on it, the USB will work without any
modifications to the kernel?
> 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.
You have a point here. I'm curious now what you reply to my question
about the possibility to modify DSDT in my previous mail. Because the
properties would be a way out of the PHY powering issue.
You know, even if we can't make changes to the DSDT (like I suspect)
we can still use the properties once this is accepted:
http://www.spinics.net/lists/linux-acpi/msg55337.html
So we can pass the "ulpi,vendor" and "ulpi,product" also with these
generic properties. I'm attaching a patch where I'm converting the
platform data AMD uses to them as an example how they could be used.
> > > 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
How can it work with mainline if there was nothing toggling the gpios
yet?
> 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).
We really can't go back to what we had. Please keep in mind that it
tied us to the USB PHY framework, possibly forever, and we shouldn't
have to depend on two different PHY frameworks. If we have to register
the PHY device in dwc3-pci.c then you should create new nop phy for
the generic phy framework and use that instead. But before you do
that, we better be damn sure there is no way to make ulpi bus work,
and we are not there yet.
Have nice weekend guys,
--
heikki
>From 0afd47616c03d268d1c0e2ad5f4108f7f1a2401d Mon Sep 17 00:00:00 2001
From: Heikki Krogerus
Date: Mon, 12 Jan 2015 16:15:10 +0200
Subject: [PATCH] usb: dwc3: pci: remove use of platform_data
This replaces the platform_data used on AMD Nolan SoC with
generic device property.
Signed-off-by: Heikki Krogerus
---
drivers/usb/dwc3/dwc3-pci.c | 53 -
1 file changed, 28 insertions(+), 25 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 8d95056..0f20139 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -22,8 +22,6 @@
#include
#include
-#include "platform_data.h"
-
/* FIXME define these in */
#define PCI_VENDOR_ID_SYNOPSYS 0x16c3
#define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB30xabcd
@@ -37,34 +35,39 @@ static int dwc3_pci_quirks(struct pci_dev *pdev)
{
if (pdev->vendor == PCI_VENDOR_ID_AMD &&
pdev->device == PCI_DEVICE_ID_AMD_NL_USB) {
- struct dwc3_platform_data pdata;
-
- memset(&pdata, 0, sizeof(pdata));
-
- pdata.has_lpm_erratum = true;
- pdata.lpm_nyet_threshold = 0xf;
-
- pdata.u2exit_lfps_quirk = true;
- pdata.u2ss_inp3_quirk = true;
- pdata.req_p1p2p3_quirk = true;
- pdata.del_p1p2p3_quirk = true;
- pdata.del_phy_power_chg_quirk = true;
- pdata.lfps_filter_quirk = true;
- pdata.rx_detect_poll_quirk = true;
-
- pdata.tx_de_emphasis_quirk = true;
- pdata.tx_de_emphasis = 1;
-
+ st
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Fri, Jan 30, 2015 at 02:18:42PM +0200, Heikki Krogerus wrote: > Hi, > > > > > 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 :/ > > So what you are saying that if I get one of those devices you > mentioned and try vanilla kernel on it, the USB will work without any > modifications to the kernel? You're misunderstanding BYT-CR SoC and external board components. The only reason that prevents most of BYT-CR boards' USB device work out-of-the-box is a switch device muxing D+/D- between host and device controllers (they depend on a single GPIO level to be toggled to get USB device working). I started discussion on how to upstream this case, but this is board related, not BYT-CR related. Some boards have also an i2c switch which requires extra i2c driver to get USB device working. But it doesn't mean the phy/otg controllers aren't fully functional with dwc3 + generic phy drivers. FWIW if you test a board without such switch (e.g. a reference BYT-T board called FFRD8 - BYT-CR is a derivation of BYT-T) it will work out-of-the-box. > > > 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. > > You have a point here. I'm curious now what you reply to my question > about the possibility to modify DSDT in my previous mail. Because the > properties would be a way out of the PHY powering issue. You missed my question. Have you tried to remove and reload dwc3 and phy modules with your test case? > > You know, even if we can't make changes to the DSDT (like I suspect) > we can still use the properties once this is accepted: > http://www.spinics.net/lists/linux-acpi/msg55337.html > > So we can pass the "ulpi,vendor" and "ulpi,product" also with these > generic properties. I'm attaching a patch where I'm converting the > platform data AMD uses to them as an example how they could be used. > > > > > 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 > > How can it work with mainline if there was nothing toggling the gpios > yet? Again, that's board related, not BYT-CR. See my reply above and redo your tests using BYT-T reference board. > > > 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). > > We really can't go back to what we had. Please keep in mind that it > tied us to the USB PHY framework, possibly forever, and we shouldn't > have to depend on two different PHY frameworks. If we have to register > the PHY device in dwc3-pci.c then you should create new nop phy for > the generic phy framework and use that instead. But before you do > that, we better be damn sure there is no way to make ulpi bus work, > and we are not there yet. You have a point. But the transition should happen without creating regressions. You cannot remove previous design while we don't have the next one merged and functional. > > > Have nice weekend guys, > You too :) Br, David > -- > heikki > From 0afd47616c03d268d1c0e2ad5f4108f7f1a2401d Mon Sep 17 00:00:00 2001 > From: Heikki Krogerus > Date: Mon, 12 Jan 2015 16:15:10 +0200 > Subject: [PATCH] usb: dwc3: pci: remove use of platform_data > > This replaces the platform_data used on AMD Nolan SoC with > generic device pr
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
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(rese
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
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.
That's a really wrong assumption. ULPI bus depends on dwc3 to be
functional and dwc3 depends on phy to be functional to complete its
power on sequence. We can't ask BIOS to let both up and running all the
time.
FWIW we *cannot* rely on ULPI bus enumeration to probe ULPI devices,
because it requires the ULPI device to be previously functional which
can't happen before the enumeration. Even if we ask BIOS to let phy
functional after boot, what happens when we remove modules and load it
again? Should we ask BIOS to power on the components again in order to
probe and power it on? It's a circular dependency you're creating.
>
> > > 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 ?
>
> Yes.
And that's wrong assumption. Not all fw will power on PHY. Maybe we
should stop all of other discussions and concentrate on this one:
BIOS will not guarantee phy will be functional after boot. And if we
remove modules and load again, it won't be functional regardless what
BIOS did during boot.
>
> > > 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
> 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.
In order for phy to be functional, it does not depend only on toggling
GPIOs. It depends on DWC3 going to reset state, then phy executes power
on sequence, then DWC3 going out of reset state to sync clocks with phy.
You're saying we should tell BIOS is concurrently mess with dwc3
together with dwc3 driver?
>
> > > 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_va
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
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()
> {
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
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 unfor
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Fri, Jan 30, 2015 at 08:20:38AM -0800, David Cohen 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. > > That's a really wrong assumption. ULPI bus depends on dwc3 to be > functional and dwc3 depends on phy to be functional to complete its > power on sequence. We can't ask BIOS to let both up and running all the > time. > > FWIW we *cannot* rely on ULPI bus enumeration to probe ULPI devices, > because it requires the ULPI device to be previously functional which > can't happen before the enumeration. Even if we ask BIOS to let phy > functional after boot, what happens when we remove modules and load it > again? Should we ask BIOS to power on the components again in order to > probe and power it on? It's a circular dependency you're creating. do we need both CS and RESET for phy to be functional ? Since we need PHY functional during ulpi bus enumeration phase, the only way would be to have the ULPI bus code itself grab those GPIOs (as long as it's gpiod_get_optional() we should be fine) and toggle them before enumerating the bus. The only problem is doing that for every driver_register() :-s -- balbi signature.asc Description: Digital signature
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
Hi David, > > > > > 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 :/ > > > > So what you are saying that if I get one of those devices you > > mentioned and try vanilla kernel on it, the USB will work without any > > modifications to the kernel? > > You're misunderstanding BYT-CR SoC and external board components. The > only reason that prevents most of BYT-CR boards' USB device work > out-of-the-box is a switch device muxing D+/D- between host and device > controllers (they depend on a single GPIO level to be toggled to get USB > device working). I started discussion on how to upstream this case, but > this is board related, not BYT-CR related. Some boards have also an i2c > switch which requires extra i2c driver to get USB device working. But it > doesn't mean the phy/otg controllers aren't fully functional with dwc3 + > generic phy drivers. OK, so after we add driver for the mux, are you saying that USB device mode works without need for any patches to dwc3 or the nop phy driver for example with v3.18? In the code that we had in v3.18, the nop phy platform data had the reset gpio value set to -1 (invalid) by the dwc3-pci, so there was nothing toggling the reset gpio yet and the cs gpio was not handled at all. So unless you are saying we don't need to toggle the gpios before the USB became operational, you would have had to patch both dwc3-pci and phy-generic in order to get it operational. And of course if we didn't need to toggle them, there would not be any need for the nop phy at all. > FWIW if you test a board without such switch (e.g. a reference BYT-T > board called FFRD8 - BYT-CR is a derivation of BYT-T) it will work > out-of-the-box. And it continues to work out-of-the-box even after we removed the creation of the PHY platform device because it does not need to toggle the gpios, right? BYT-T boards are actually one of the reason why we would really need the ulpi bus, because most them have tusb1211 (so not tusb1210) as the phy and they use it to detect the charger among other things. > You missed my question. Have you tried to remove and reload dwc3 and phy > modules with your test case? I do test unloading all the modules and reloading them back every time, and with the hack I suggested everything works just fine. > > We really can't go back to what we had. Please keep in mind that it > > tied us to the USB PHY framework, possibly forever, and we shouldn't > > have to depend on two different PHY frameworks. If we have to register > > the PHY device in dwc3-pci.c then you should create new nop phy for > > the generic phy framework and use that instead. But before you do > > that, we better be damn sure there is no way to make ulpi bus work, > > and we are not there yet. > > You have a point. But the transition should happen without creating > regressions. You cannot remove previous design while we don't have the > next one merged and functional. But I still don't see any regressions? Thanks, -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
> > > > 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. > > That's a really wrong assumption. ULPI bus depends on dwc3 to be > functional and dwc3 depends on phy to be functional to complete its > power on sequence. We can't ask BIOS to let both up and running all the > time. > > FWIW we *cannot* rely on ULPI bus enumeration to probe ULPI devices, > because it requires the ULPI device to be previously functional which > can't happen before the enumeration. Even if we ask BIOS to let phy > functional after boot, what happens when we remove modules and load it > again? Should we ask BIOS to power on the components again in order to > probe and power it on? It's a circular dependency you're creating. > > > > > > > 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 ? > > > > Yes. > > And that's wrong assumption. Not all fw will power on PHY. Maybe we > should stop all of other discussions and concentrate on this one: > BIOS will not guarantee phy will be functional after boot. And if we > remove modules and load again, it won't be functional regardless what > BIOS did during boot. I was wrong when I talked about BIOS powering the PHY before bootup. I admit it. I'm saying this again as a reminder to myself: We can't mix BIOS (or any FW) and ACPI. I mix them constantly. And I definitely need to stop talking about bootup. How this is handled is by letting the ACPI Power State methods of the dwc3 device take care of the toggling of the gpios. It is done with the help of something called GPIO OperationRegion. In case you are not familiar with ACPI, then if you send me ACPI dump of one of those devices (or just copy /sys/firmware/acpi/tables/DSDT), I can try to modify the DSDT for you so you can use to test it, and provide you with the ASL snippet. You will see that the PHY is indeed in reset after bootup like it is now (BIOS does nothing differently), but the gpios are automatically toggled by the DSDT code. So every time you load dwc3 module, the PHY will be operational when we need it, and when you unload dwc3, it will be left in reset again. The OS does not have to do anything. I really think that this BYT-CR case will be one of really rare exception we will see, especially if we manage to put together the ACPI table review that has been though about. > > > > 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 > > 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. > > In order for phy to be functional, it does not depend only on toggling > GPIOs. It depends on DWC3 going to reset state, then phy executes power > on sequence, then DWC3 going out of reset state to sync clocks with phy. > You're saying we should tell BIOS is concurrently mess with dwc3 > together with dwc3 driver? I don't understand
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
Hi David, Felipe, > > > > 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. > > > > Doesn't the ugly platform device case look less ugly than this? > > The platform device would in any case need to be created only for this > case. We can't any more just create the phy device unconditionally for > every PCI platform like it was created before, as it's clear now the > PHY device can be be created from other sources. It was a risk always. > > But the big problem is that since the PHY on your board is ULPI PHY, > it will make it really difficult to add the ULPI bus support. And like > I said in my previous mail, we would really need it for the boards > that expect the PHY driver to provide functions like charger > detection. We don't need it only for BYT based boards. > > So on top of the above, since the GPIO resources are given to dwc3, I > actually think that my hack is better then the platform device. So what do you guys think we should do? I can't figure out how would we make the platform device work with ulpi bus. If we think about handling this in ulpi bus driver by setting setting the gpios before attempting to access the phy, which I'm not completely against, we have couple of problems. Firstly, the gpio resources are given to the dwc3 in this case, while normally they will be given to separate device object for the phy. Secondly, these gpios were not labeled in DSDT, but apparently requesting the gpios with index will be deprecated and not acceptable any more. With the remaining platforms that have not labeled the gpios we have to bind the gpios to labels separately in the drivers. With ACPI platforms the labels are introduced in struct acpi_gpio_mapping which needs to be registered with acpi_dev_add_driver_gpios(). Check net/rfkill/rfkill-gpio.c as an example how to use it. I think those points would make this too platform specific case to be handled in the ulpi bus driver. Suggestions? I still think the correct thing to do is to handle this in the quirk in dwc3-pci.c. Cheers, -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Tue, Feb 03, 2015 at 01:37:39PM +0200, Heikki Krogerus wrote: > Hi David, Felipe, Hi Heikki, > > > > > > 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. > > > > > > Doesn't the ugly platform device case look less ugly than this? > > > > The platform device would in any case need to be created only for this > > case. We can't any more just create the phy device unconditionally for > > every PCI platform like it was created before, as it's clear now the > > PHY device can be be created from other sources. It was a risk always. > > > > But the big problem is that since the PHY on your board is ULPI PHY, > > it will make it really difficult to add the ULPI bus support. And like > > I said in my previous mail, we would really need it for the boards > > that expect the PHY driver to provide functions like charger > > detection. We don't need it only for BYT based boards. > > > > So on top of the above, since the GPIO resources are given to dwc3, I > > actually think that my hack is better then the platform device. > > So what do you guys think we should do? I can't figure out how would > we make the platform device work with ulpi bus. If we think about > handling this in ulpi bus driver by setting setting the gpios before > attempting to access the phy, which I'm not completely against, we > have couple of problems. It's still not enough :/ In order to make the phy functional (in BYT case) you need to bring DWC3 to reset state, then toggle these gpios to reset and power on phy, then remove DWC3 from reset state so both can sync clocks. You can still reset and power on phy before DWC3 goes to and from reset, but then there's a chance phy may become unstable. It's then expected to fail long stability tests. > > Firstly, the gpio resources are given to the dwc3 in this case, > while normally they will be given to separate device object for the > phy. Yup. You're correct. We can influence new products to not repeat this issue, but we still need to support legacy ones. > > Secondly, these gpios were not labeled in DSDT, but apparently > requesting the gpios with index will be deprecated and not acceptable > any more. With the remaining platforms that have not labeled the gpios > we have to bind the gpios to labels separately in the drivers. With > ACPI platforms the labels are introduced in struct acpi_gpio_mapping > which needs to be registered with acpi_dev_add_driver_gpios(). Check > net/rfkill/rfkill-gpio.c as an example how to use it. For new products, certanly. > > I think those points would make this too platform specific case to be > handled in the ulpi bus driver. > > Suggestions? I still think the correct thing to do is to handle this > in the quirk in dwc3-pci.c. IMO it would make things uglier than it was before. BR, David > > > Cheers, > > -- > heikki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Mon, Feb 02, 2015 at 02:59:59PM +0200, Heikki Krogerus wrote: > > > > > 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. > > > > That's a really wrong assumption. ULPI bus depends on dwc3 to be > > functional and dwc3 depends on phy to be functional to complete its > > power on sequence. We can't ask BIOS to let both up and running all the > > time. > > > > FWIW we *cannot* rely on ULPI bus enumeration to probe ULPI devices, > > because it requires the ULPI device to be previously functional which > > can't happen before the enumeration. Even if we ask BIOS to let phy > > functional after boot, what happens when we remove modules and load it > > again? Should we ask BIOS to power on the components again in order to > > probe and power it on? It's a circular dependency you're creating. > > > > > > > > > > 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 ? > > > > > > Yes. > > > > And that's wrong assumption. Not all fw will power on PHY. Maybe we > > should stop all of other discussions and concentrate on this one: > > BIOS will not guarantee phy will be functional after boot. And if we > > remove modules and load again, it won't be functional regardless what > > BIOS did during boot. > > I was wrong when I talked about BIOS powering the PHY before bootup. I > admit it. I'm saying this again as a reminder to myself: We can't mix > BIOS (or any FW) and ACPI. I mix them constantly. And I definitely > need to stop talking about bootup. > > How this is handled is by letting the ACPI Power State methods of the > dwc3 device take care of the toggling of the gpios. It is done with > the help of something called GPIO OperationRegion. In case you are not > familiar with ACPI, then if you send me ACPI dump of one of those > devices (or just copy /sys/firmware/acpi/tables/DSDT), I can try to > modify the DSDT for you so you can use to test it, and provide you > with the ASL snippet. > > You will see that the PHY is indeed in reset after bootup like it is > now (BIOS does nothing differently), but the gpios are automatically > toggled by the DSDT code. So every time you load dwc3 module, the PHY > will be operational when we need it, and when you unload dwc3, it will > be left in reset again. The OS does not have to do anything. Hm. That changes everything :) It's a feasible direction to push vendors. > > I really think that this BYT-CR case will be one of really rare > exception we will see, especially if we manage to put together the > ACPI table review that has been though about. I hope it will. > > > > > > 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 > > > 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. > >
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Tue, Feb 10, 2015 at 11:05:31AM -0800, David Cohen wrote: > On Mon, Feb 02, 2015 at 02:59:59PM +0200, Heikki Krogerus wrote: > > > > > > 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. > > > > > > That's a really wrong assumption. ULPI bus depends on dwc3 to be > > > functional and dwc3 depends on phy to be functional to complete its > > > power on sequence. We can't ask BIOS to let both up and running all the > > > time. > > > > > > FWIW we *cannot* rely on ULPI bus enumeration to probe ULPI devices, > > > because it requires the ULPI device to be previously functional which > > > can't happen before the enumeration. Even if we ask BIOS to let phy > > > functional after boot, what happens when we remove modules and load it > > > again? Should we ask BIOS to power on the components again in order to > > > probe and power it on? It's a circular dependency you're creating. > > > > > > > > > > > > > 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 ? > > > > > > > > Yes. > > > > > > And that's wrong assumption. Not all fw will power on PHY. Maybe we > > > should stop all of other discussions and concentrate on this one: > > > BIOS will not guarantee phy will be functional after boot. And if we > > > remove modules and load again, it won't be functional regardless what > > > BIOS did during boot. > > > > I was wrong when I talked about BIOS powering the PHY before bootup. I > > admit it. I'm saying this again as a reminder to myself: We can't mix > > BIOS (or any FW) and ACPI. I mix them constantly. And I definitely > > need to stop talking about bootup. > > > > How this is handled is by letting the ACPI Power State methods of the > > dwc3 device take care of the toggling of the gpios. It is done with > > the help of something called GPIO OperationRegion. In case you are not > > familiar with ACPI, then if you send me ACPI dump of one of those > > devices (or just copy /sys/firmware/acpi/tables/DSDT), I can try to > > modify the DSDT for you so you can use to test it, and provide you > > with the ASL snippet. > > > > You will see that the PHY is indeed in reset after bootup like it is > > now (BIOS does nothing differently), but the gpios are automatically > > toggled by the DSDT code. So every time you load dwc3 module, the PHY > > will be operational when we need it, and when you unload dwc3, it will > > be left in reset again. The OS does not have to do anything. > > Hm. That changes everything :) > It's a feasible direction to push vendors. > > > > > I really think that this BYT-CR case will be one of really rare > > exception we will see, especially if we manage to put together the > > ACPI table review that has been though about. > > I hope it will. > > > > > > > > > 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 > > > > 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 consi
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
Hi David, > > > > In order for phy to be functional, it does not depend only on toggling > > > > GPIOs. It depends on DWC3 going to reset state, then phy executes power > > > > on sequence, then DWC3 going out of reset state to sync clocks with phy. > > > > You're saying we should tell BIOS is concurrently mess with dwc3 > > > > together with dwc3 driver? > > > > > > I don't understand what you are saying here? > > > > TUSB1210 needs to come out of reset only when DWC3 is in reset state. > > This is how current code works in dwc3_core_soft_reset(): > > - dwc3 goes to reset > > - phy goes to reset > > - phy gets out of reset > > - dwc3 gets out of reset > > > > This is how you're proposing: > > - phy goes to reset (DSDT code, when loading module) > > - phy gets out of reset (DSDT code, when loading module) > > > > - dwc3 goes to reset (dwc3_core_soft_reset()) > > - dwc3 gets our of reset (dwc3_core_soft_reset()) > > > > Felipe, do you see a problem with this new context? If not, I'm > > satisfied with Heikki's ULPI bus proposal considering my comment below. > > Sorry, guess I spoke too soon :/ > I am satisfied with the phy case, but I forgot about the chicken/egg > problem I reported earlier: > DWC3 will not be functional when reloading the module after it went to > reset state. Then ULPI enumeration can't happen regardless DSDT code > powered on phy. One point here. If we have DSDT handling the gpios with the operation region, those gpio resources don't need to be given to any device (actually I think they really shouldn't be given to anything in that case). > Heikki, do you have a proposal for that? IMHO that's the main missing > point if we forget about BYT-CR legacy. I'm sorry but I'm still not sure about the scenario you are talking about. When we load dwc3, we end up autoloading phy-tusb1210 in this case and increasing the phy devices ref count i.e. preventing phy-tusb1210 module from being unloaded before dwc3 is unloaded. If we unload dwc3 we can also unload phy-tusb1210 if we like but if after that we load dwc3 again, the ULPI will be accessible the moment we register the ulpi interface as it was before. That I believe is actually a must in case of ULPI. When dwc3 is reset with GCTL or DCTL SoftReset, it will first write to the ULPI FunctionControl register's reset bit in order to but the PHY to reset (PHYSoftRst has no effect in case of ULPI), so ULPI really has to be accessible before the core is soft reset. Thanks, -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Wed, Feb 11, 2015 at 03:12:55PM +0200, Heikki Krogerus wrote: > Hi David, Hi Heikki, > > > > > > In order for phy to be functional, it does not depend only on toggling > > > > > GPIOs. It depends on DWC3 going to reset state, then phy executes > > > > > power > > > > > on sequence, then DWC3 going out of reset state to sync clocks with > > > > > phy. > > > > > You're saying we should tell BIOS is concurrently mess with dwc3 > > > > > together with dwc3 driver? > > > > > > > > I don't understand what you are saying here? > > > > > > TUSB1210 needs to come out of reset only when DWC3 is in reset state. > > > This is how current code works in dwc3_core_soft_reset(): > > > - dwc3 goes to reset > > > - phy goes to reset > > > - phy gets out of reset > > > - dwc3 gets out of reset > > > > > > This is how you're proposing: > > > - phy goes to reset (DSDT code, when loading module) > > > - phy gets out of reset (DSDT code, when loading module) > > > > > > - dwc3 goes to reset (dwc3_core_soft_reset()) > > > - dwc3 gets our of reset (dwc3_core_soft_reset()) > > > > > > Felipe, do you see a problem with this new context? If not, I'm > > > satisfied with Heikki's ULPI bus proposal considering my comment below. > > > > Sorry, guess I spoke too soon :/ > > I am satisfied with the phy case, but I forgot about the chicken/egg > > problem I reported earlier: > > DWC3 will not be functional when reloading the module after it went to > > reset state. Then ULPI enumeration can't happen regardless DSDT code > > powered on phy. > > One point here. If we have DSDT handling the gpios with the operation > region, those gpio resources don't need to be given to any device > (actually I think they really shouldn't be given to anything in that > case). Agree with that. > > > Heikki, do you have a proposal for that? IMHO that's the main missing > > point if we forget about BYT-CR legacy. > > I'm sorry but I'm still not sure about the scenario you are talking > about. Probably because I'm asking this question in the wrong place. It is regarding patch 6/8. I resent the question there. Br, David -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
Hi Heikki, Sorry I am starting a new branch on this thread. I need to go back to another topic on this same patch. On Fri, Jan 23, 2015 at 05:12:58PM +0200, Heikki Krogerus wrote: > TUSB1210 ULPI PHY has vendor specific register for eye > diagram tuning. On some platforms the system firmware has > set optimized value to it. In order to not loose the > optimized value, the driver stores it during probe and > restores it every time the PHY is powered back on. > > Signed-off-by: Heikki Krogerus > Cc: Kishon Vijay Abraham I > --- [snip] > + /* Store initial eye diagram optimisation value */ > + ret = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2); We can't rely on eye diagram optimization value here. It's possible the phy went through reset (e.g. module unloading/loading). We need a more consistent method. Could we use _DSD instead? That would be more compatible with DT too. Br, David > + if (ret < 0) > + return ret; > + > + tusb->eye_diagram_tuning = ret; > + > + tusb->phy = ulpi_phy_create(ulpi, &phy_ops); > + if (IS_ERR(tusb->phy)) > + return PTR_ERR(tusb->phy); > + > + tusb->ulpi = ulpi; > + > + phy_set_drvdata(tusb->phy, tusb); > + ulpi_set_drvdata(ulpi, tusb); > + return 0; > +} > + -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
Hi, On Thu, Feb 12, 2015 at 05:52:42PM -0800, David Cohen wrote: > Hi Heikki, > > Sorry I am starting a new branch on this thread. > I need to go back to another topic on this same patch. > > On Fri, Jan 23, 2015 at 05:12:58PM +0200, Heikki Krogerus wrote: > > TUSB1210 ULPI PHY has vendor specific register for eye > > diagram tuning. On some platforms the system firmware has > > set optimized value to it. In order to not loose the > > optimized value, the driver stores it during probe and > > restores it every time the PHY is powered back on. > > > > Signed-off-by: Heikki Krogerus > > Cc: Kishon Vijay Abraham I > > --- > > [snip] > > > + /* Store initial eye diagram optimisation value */ > > + ret = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2); > > We can't rely on eye diagram optimization value here. It's possible the > phy went through reset (e.g. module unloading/loading). > We need a more consistent method. Could we use _DSD instead? That would > be more compatible with DT too. I'm don't have any problem with getting the value from a property. Sounds like the correct thing to do. Are you thinking about putting that _DSD method to the ACPI device object for the dwc3? The correct place would probable be separate device object for the PHY, but if we do that we need to consider how that object is bound to the ulpi device. There are few ways we can do that, so it requires some thinking. In any case this driver we can already implement so that it expects to get the value from property. We just need the name for it. The register has actually separate fields for High speed output impedance configuration and High speed output driver strength configuration for eye diagram tuning, plus control bit for DP/DM swap. Maybe we should actually get them from three separate properties: device_property_read_u8(dev, "datapolarity", &val); ... device_property_read_u8(dev, "zhsdrv", &val); ... device_property_read_u8(dev, "ihstx", &val); ... What do you think? Cheers, -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Fri, Feb 13, 2015 at 02:35:16PM +0200, Heikki Krogerus wrote: > Hi, > > On Thu, Feb 12, 2015 at 05:52:42PM -0800, David Cohen wrote: > > Hi Heikki, > > > > Sorry I am starting a new branch on this thread. > > I need to go back to another topic on this same patch. > > > > On Fri, Jan 23, 2015 at 05:12:58PM +0200, Heikki Krogerus wrote: > > > TUSB1210 ULPI PHY has vendor specific register for eye > > > diagram tuning. On some platforms the system firmware has > > > set optimized value to it. In order to not loose the > > > optimized value, the driver stores it during probe and > > > restores it every time the PHY is powered back on. > > > > > > Signed-off-by: Heikki Krogerus > > > Cc: Kishon Vijay Abraham I > > > --- > > > > [snip] > > > > > + /* Store initial eye diagram optimisation value */ > > > + ret = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2); > > > > We can't rely on eye diagram optimization value here. It's possible the > > phy went through reset (e.g. module unloading/loading). > > We need a more consistent method. Could we use _DSD instead? That would > > be more compatible with DT too. > > I'm don't have any problem with getting the value from a property. > Sounds like the correct thing to do. Are you thinking about putting > that _DSD method to the ACPI device object for the dwc3? The correct > place would probable be separate device object for the PHY, but if we > do that we need to consider how that object is bound to the ulpi > device. There are few ways we can do that, so it requires some > thinking. > > In any case this driver we can already implement so that it expects to > get the value from property. We just need the name for it. > > The register has actually separate fields for High speed output > impedance configuration and High speed output driver strength > configuration for eye diagram tuning, plus control bit for DP/DM swap. > > Maybe we should actually get them from three separate properties: > > device_property_read_u8(dev, "datapolarity", &val); > ... > device_property_read_u8(dev, "zhsdrv", &val); > ... > device_property_read_u8(dev, "ihstx", &val); > ... > > What do you think? it's probably better to pass each value and let the driver calculate the register contents like this. -- balbi signature.asc Description: Digital signature
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
Hi Felipe,
[snip]
> 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);
> + }
> + }
> +
A lot has been discussed in other branches of this thread.
But in resume, this is the last open point to make Heikki's proposal
good on my side. If you accept this ugly quirk (but necessary for
current BYT-CR products when ULPI bus enumerates phy), everything seems
good to me. If you don't accept, we need to figure out a way to get the
platform driver code back to give gpio to phy as platform data in a way
that it could live together with ULPI bus (BYT-CR needs the ULPI bus too).
Br, David
> if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> pdev->device == PCI_DEVICE_ID_AMD_NL_USB) {
> struct dwc3_platform_data pdata;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Fri, Feb 13, 2015 at 02:02:11PM -0800, David Cohen wrote:
> Hi Felipe,
>
> [snip]
>
> > 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);
> > + }
> > + }
> > +
>
> A lot has been discussed in other branches of this thread.
>
> But in resume, this is the last open point to make Heikki's proposal
> good on my side. If you accept this ugly quirk (but necessary for
> current BYT-CR products when ULPI bus enumerates phy), everything seems
> good to me. If you don't accept, we need to figure out a way to get the
> platform driver code back to give gpio to phy as platform data in a way
> that it could live together with ULPI bus (BYT-CR needs the ULPI bus too).
Is this needed to *all* Baytrail devices or could we have devices with
updated firmware which won't need this ? I wonder if we should apply the
quirk for each known product that actually needs this.
Also, I will only accept the series, after I'm shown logs that it works
with your known-to-be-broken device.
cheers
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY
On Fri, Feb 13, 2015 at 04:03:57PM -0600, Felipe Balbi wrote:
> On Fri, Feb 13, 2015 at 02:02:11PM -0800, David Cohen wrote:
> > Hi Felipe,
> >
> > [snip]
> >
> > > 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);
> > > + }
> > > + }
> > > +
> >
> > A lot has been discussed in other branches of this thread.
> >
> > But in resume, this is the last open point to make Heikki's proposal
> > good on my side. If you accept this ugly quirk (but necessary for
> > current BYT-CR products when ULPI bus enumerates phy), everything seems
> > good to me. If you don't accept, we need to figure out a way to get the
> > platform driver code back to give gpio to phy as platform data in a way
> > that it could live together with ULPI bus (BYT-CR needs the ULPI bus too).
>
> Is this needed to *all* Baytrail devices or could we have devices with
> updated firmware which won't need this ? I wonder if we should apply the
> quirk for each known product that actually needs this.
The products that need this quirk will have the gpios on DSDT, otherwise
it won't be there. Except for the order of gpios (CS needs to be enabled
first and it's index 0 AFAIR), the quirk should follow Heikki's logic
here: if gpio isn't found we silently ignore it.
>
> Also, I will only accept the series, after I'm shown logs that it works
> with your known-to-be-broken device.
I can provide that when Heikki resends his patch set.
Br, David
>
> cheers
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
