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

[Felipe Balbi <balbi@ti.com>]

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

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 <heikki.krogerus@linux.intel.com>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  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 0000000..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 <heikki.krogerus@linux.intel.com>
> + *
> + * 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 <linux/module.h>
> +#include <linux/ulpi/driver.h>
> +#include <linux/gpio/consumer.h>
> +
> +#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 diagram data), or remove it from here
completely and use the BIOS hook which, apparently, exists.

Wrapping it with GPIOs has the added benefit of making sure the PHY is
powered down after probe in case firmware left it running. We really
can't make assumptions as to when the PHY will be needed.

-- 
balbi

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