[PATCH 0/8] usb: ulpi bus

[PATCH 0/8] usb: ulpi bus

[Heikki Krogerus]

Heikki Krogerus (8):
  usb: add bus type for USB ULPI
  usb: dwc3: USB2 PHY register access bits
  usb: dwc3: store driver data earlier
  usb: dwc3: cache hwparams earlier
  usb: dwc3: ULPI or UTMI+ select
  usb: dwc3: add ULPI interface support
  phy: helpers for USB ULPI PHY registering
  phy: add driver for TI TUSB1210 ULPI PHY

 MAINTAINERS                       |   7 ++
 drivers/phy/Kconfig               |   6 +
 drivers/phy/Makefile              |   1 +
 drivers/phy/phy-tusb1210.c        | 133 ++++++++++++++++++++
 drivers/phy/ulpi_phy.h            |  31 +++++
 drivers/usb/common/Makefile       |   3 +-
 drivers/usb/common/ulpi.c         | 253 ++++++++++++++++++++++++++++++++++++++
 drivers/usb/core/Kconfig          |   8 ++
 drivers/usb/dwc3/Kconfig          |   7 ++
 drivers/usb/dwc3/Makefile         |   4 +
 drivers/usb/dwc3/core.c           |  12 +-
 drivers/usb/dwc3/core.h           |  22 ++++
 drivers/usb/dwc3/ulpi.c           | 112 +++++++++++++++++
 include/linux/mod_devicetable.h   |   6 +
 include/linux/ulpi/driver.h       |  62 ++++++++++
 include/linux/ulpi/interface.h    |  23 ++++
 include/linux/ulpi/regs.h         | 130 ++++++++++++++++++++
 include/linux/usb/ulpi.h          | 134 +-------------------
 scripts/mod/devicetable-offsets.c |   4 +
 scripts/mod/file2alias.c          |  13 ++
 20 files changed, 835 insertions(+), 136 deletions(-)
 create mode 100644 drivers/phy/phy-tusb1210.c
 create mode 100644 drivers/phy/ulpi_phy.h
 create mode 100644 drivers/usb/common/ulpi.c
 create mode 100644 drivers/usb/dwc3/ulpi.c
 create mode 100644 include/linux/ulpi/driver.h
 create mode 100644 include/linux/ulpi/interface.h
 create mode 100644 include/linux/ulpi/regs.h

-- 
2.1.4

[PATCH 1/8] usb: add bus type for USB ULPI

[Heikki Krogerus]

UTMI+ Low Pin Interface (ULPI) is a commonly used PHY
interface for USB 2.0. The ULPI specification describes a
standard set of registers which the vendors can extend for
their specific needs. ULPI PHYs provide often functions
such as charger detection and ADP sensing and probing.

There are two major issues that the bus type is meant to
tackle:

Firstly, ULPI registers are accessed from the controller.
The bus provides convenient method for the controller
drivers to share that access with the actual PHY drivers.

Secondly, there are already platforms that assume ULPI PHYs
are runtime detected, such as many Intel Baytrail based
platforms. They do not provide any kind of hardware
description for the ULPI PHYs like separate ACPI device
object that could be used to enumerate a device from.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 MAINTAINERS                       |   7 ++
 drivers/usb/common/Makefile       |   3 +-
 drivers/usb/common/ulpi.c         | 253 ++++++++++++++++++++++++++++++++++++++
 drivers/usb/core/Kconfig          |   8 ++
 include/linux/mod_devicetable.h   |   6 +
 include/linux/ulpi/driver.h       |  62 ++++++++++
 include/linux/ulpi/interface.h    |  23 ++++
 include/linux/ulpi/regs.h         | 130 ++++++++++++++++++++
 include/linux/usb/ulpi.h          | 134 +-------------------
 scripts/mod/devicetable-offsets.c |   4 +
 scripts/mod/file2alias.c          |  13 ++
 11 files changed, 510 insertions(+), 133 deletions(-)
 create mode 100644 drivers/usb/common/ulpi.c
 create mode 100644 include/linux/ulpi/driver.h
 create mode 100644 include/linux/ulpi/interface.h
 create mode 100644 include/linux/ulpi/regs.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 761a4a1..7d0c58c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10234,6 +10234,13 @@ S:	Maintained
 F:	Documentation/video4linux/zr364xx.txt
 F:	drivers/media/usb/zr364xx/
 
+ULPI BUS
+M:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+L:	linux-usb@vger.kernel.org
+S:	Maintained
+F:	drivers/usb/common/ulpi.c
+F:	include/linux/ulpi/
+
 USER-MODE LINUX (UML)
 M:	Jeff Dike <jdike@addtoit.com>
 M:	Richard Weinberger <richard@nod.at>
diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
index ca2f8bd..5791d6c 100644
--- a/drivers/usb/common/Makefile
+++ b/drivers/usb/common/Makefile
@@ -6,4 +6,5 @@ obj-$(CONFIG_USB_COMMON)	  += usb-common.o
 usb-common-y			  += common.o
 usb-common-$(CONFIG_USB_LED_TRIG) += led.o
 
-obj-$(CONFIG_USB_OTG_FSM) += usb-otg-fsm.o
+obj-$(CONFIG_USB_OTG_FSM)	+= usb-otg-fsm.o
+obj-$(CONFIG_USB_ULPI_BUS)	+= ulpi.o
diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
new file mode 100644
index 0000000..3ed66be
--- /dev/null
+++ b/drivers/usb/common/ulpi.c
@@ -0,0 +1,253 @@
+/**
+ * ulpi.c - USB ULPI PHY bus
+ *
+ * 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/ulpi/interface.h>
+#include <linux/ulpi/driver.h>
+#include <linux/ulpi/regs.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+/* -------------------------------------------------------------------------- */
+
+int ulpi_read(struct ulpi *ulpi, u8 addr)
+{
+	return ulpi->ops->read(ulpi->ops, addr);
+}
+EXPORT_SYMBOL_GPL(ulpi_read);
+
+int ulpi_write(struct ulpi *ulpi, u8 addr, u8 val)
+{
+	return ulpi->ops->write(ulpi->ops, addr, val);
+}
+EXPORT_SYMBOL_GPL(ulpi_write);
+
+/* -------------------------------------------------------------------------- */
+
+static int ulpi_match(struct device *dev, struct device_driver *driver)
+{
+	struct ulpi_driver *drv = to_ulpi_driver(driver);
+	struct ulpi *ulpi = to_ulpi_dev(dev);
+	const struct ulpi_device_id *id;
+
+	for (id = drv->id_table; id->vendor; id++)
+		if (id->vendor == ulpi->id.vendor &&
+		    id->product == ulpi->id.product)
+			return 1;
+
+	return 0;
+}
+
+static int ulpi_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+	struct ulpi *ulpi = to_ulpi_dev(dev);
+
+	if (add_uevent_var(env, "MODALIAS=ulpi:v%04xp%04x",
+			   ulpi->id.vendor, ulpi->id.product))
+		return -ENOMEM;
+	return 0;
+}
+
+static int ulpi_probe(struct device *dev)
+{
+	struct ulpi_driver *drv = to_ulpi_driver(dev->driver);
+
+	return drv->probe(to_ulpi_dev(dev));
+}
+
+static int ulpi_remove(struct device *dev)
+{
+	struct ulpi_driver *drv = to_ulpi_driver(dev->driver);
+
+	if (drv->remove)
+		drv->remove(to_ulpi_dev(dev));
+
+	return 0;
+}
+
+struct bus_type ulpi_bus = {
+	.name = "ulpi",
+	.match = ulpi_match,
+	.uevent = ulpi_uevent,
+	.probe = ulpi_probe,
+	.remove = ulpi_remove,
+};
+EXPORT_SYMBOL_GPL(ulpi_bus);
+
+/* -------------------------------------------------------------------------- */
+
+static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct ulpi *ulpi = to_ulpi_dev(dev);
+
+	return sprintf(buf, "ulpi:v%04xp%04x\n",
+		       ulpi->id.vendor, ulpi->id.product);
+}
+static DEVICE_ATTR_RO(modalias);
+
+static struct attribute *ulpi_dev_attrs[] = {
+	&dev_attr_modalias.attr,
+	NULL
+};
+
+static struct attribute_group ulpi_dev_attr_group = {
+	.attrs = ulpi_dev_attrs,
+};
+
+static const struct attribute_group *ulpi_dev_attr_groups[] = {
+	&ulpi_dev_attr_group,
+	NULL
+};
+
+static void ulpi_dev_release(struct device *dev)
+{
+	kfree(to_ulpi_dev(dev));
+}
+
+struct device_type ulpi_device_type = {
+	.name = "ulpi_device",
+	.groups = ulpi_dev_attr_groups,
+	.release = ulpi_dev_release,
+};
+
+/* -------------------------------------------------------------------------- */
+
+/**
+ * ulpi_register_driver - register a driver with the ULPI bus
+ * @drv: driver being registered
+ *
+ * Registers a driver with the ULPI bus.
+ */
+int ulpi_register_driver(struct ulpi_driver *drv)
+{
+	if (!drv->probe)
+		return -EINVAL;
+
+	drv->driver.bus = &ulpi_bus;
+
+	return driver_register(&drv->driver);
+}
+EXPORT_SYMBOL_GPL(ulpi_register_driver);
+
+/**
+ * ulpi_register_driver - unregister a driver with the ULPI bus
+ * @drv: driver to unregister
+ *
+ * Unregisters a driver with the ULPI bus.
+ */
+void ulpi_unregister_driver(struct ulpi_driver *drv)
+{
+	driver_unregister(&drv->driver);
+}
+EXPORT_SYMBOL_GPL(ulpi_unregister_driver);
+
+/* -------------------------------------------------------------------------- */
+
+static int ulpi_register(struct device *dev, struct ulpi *ulpi)
+{
+	int ret;
+
+	/* Test the interface */
+	ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
+	if (ret < 0)
+		return ret;
+
+	ret = ulpi_read(ulpi, ULPI_SCRATCH);
+	if (ret < 0)
+		return ret;
+
+	if (ret != 0xaa)
+		return -ENODEV;
+
+	ulpi->id.vendor = ulpi_read(ulpi, ULPI_VENDOR_ID_LOW);
+	ulpi->id.vendor |= ulpi_read(ulpi, ULPI_VENDOR_ID_HIGH) << 8;
+
+	ulpi->id.product = ulpi_read(ulpi, ULPI_PRODUCT_ID_LOW);
+	ulpi->id.product |= ulpi_read(ulpi, ULPI_PRODUCT_ID_HIGH) << 8;
+
+	ulpi->dev.parent = dev;
+	ulpi->dev.bus = &ulpi_bus;
+	ulpi->dev.type = &ulpi_device_type;
+	dev_set_name(&ulpi->dev, "%s.ulpi", dev_name(dev));
+
+	request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
+
+	ret = device_register(&ulpi->dev);
+	if (ret)
+		return ret;
+
+	dev_dbg(&ulpi->dev, "registered ULPI PHY: vendor %04x, product %04x\n",
+		ulpi->id.vendor, ulpi->id.product);
+
+	return 0;
+}
+
+/**
+ * ulpi_register_interface - instantiate new ULPI device
+ * @dev: USB controller's device interface
+ * @ops: ULPI register access
+ *
+ * Allocates and registers a ULPI device and an interface for it. Called from
+ * the USB controller that provides the ULPI interface.
+ */
+struct ulpi *ulpi_register_interface(struct device *dev, struct ulpi_ops *ops)
+{
+	struct ulpi *ulpi;
+	int ret;
+
+	ulpi = kzalloc(sizeof(*ulpi), GFP_KERNEL);
+	if (!ulpi)
+		return ERR_PTR(-ENOMEM);
+
+	ulpi->ops = ops;
+	ops->dev = dev;
+
+	ret = ulpi_register(dev, ulpi);
+	if (ret) {
+		kfree(ulpi);
+		return ERR_PTR(ret);
+	}
+
+	return ulpi;
+}
+EXPORT_SYMBOL_GPL(ulpi_register_interface);
+
+/**
+ * ulpi_unregister_interface - unregister ULPI interface
+ * @intrf: struct ulpi_interface
+ *
+ * Unregisters a ULPI device and it's interface that was created with
+ * ulpi_create_interface().
+ */
+void ulpi_unregister_interface(struct ulpi *ulpi)
+{
+	device_unregister(&ulpi->dev);
+}
+EXPORT_SYMBOL_GPL(ulpi_unregister_interface);
+
+/* -------------------------------------------------------------------------- */
+
+static int __init ulpi_init(void)
+{
+	return bus_register(&ulpi_bus);
+}
+module_init(ulpi_init);
+
+static void __exit ulpi_exit(void)
+{
+	bus_unregister(&ulpi_bus);
+}
+module_exit(ulpi_exit);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("USB ULPI PHY bus");
diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
index cc0ced0..c0fc3b5 100644
--- a/drivers/usb/core/Kconfig
+++ b/drivers/usb/core/Kconfig
@@ -84,3 +84,11 @@ config USB_OTG_FSM
 	  Implements OTG Finite State Machine as specified in On-The-Go
 	  and Embedded Host Supplement to the USB Revision 2.0 Specification.
 
+config USB_ULPI_BUS
+	bool "USB ULPI PHY interface support"
+	depends on USB || USB_GADGET
+	help
+	  Say yes if you have ULPI PHY attached to your USB controller.
+
+	  If unsure, say N.
+
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 745def8..6581615 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -626,4 +626,10 @@ struct mcb_device_id {
 	kernel_ulong_t driver_data;
 };
 
+struct ulpi_device_id {
+	__u16 vendor;
+	__u16 product;
+	kernel_ulong_t driver_data;
+};
+
 #endif /* LINUX_MOD_DEVICETABLE_H */
diff --git a/include/linux/ulpi/driver.h b/include/linux/ulpi/driver.h
new file mode 100644
index 0000000..a90d216
--- /dev/null
+++ b/include/linux/ulpi/driver.h
@@ -0,0 +1,62 @@
+#ifndef __LINUX_ULPI_DRIVER_H
+#define __LINUX_ULPI_DRIVER_H
+
+#include <linux/mod_devicetable.h>
+
+#include <linux/device.h>
+
+struct ulpi_ops;
+
+/**
+ * struct ulpi - describes ULPI PHY device
+ * @id: vendor and product ids for ULPI device
+ * @ops: I/O access
+ * @dev: device interface
+ */
+struct ulpi {
+	struct ulpi_device_id id;
+	struct ulpi_ops *ops;
+	struct device dev;
+};
+
+#define to_ulpi_dev(d) container_of(d, struct ulpi, dev)
+
+static inline void ulpi_set_drvdata(struct ulpi *ulpi, void *data)
+{
+	dev_set_drvdata(&ulpi->dev, data);
+}
+
+static inline void *ulpi_get_drvdata(struct ulpi *ulpi)
+{
+	return dev_get_drvdata(&ulpi->dev);
+}
+
+/**
+ * struct ulpi_driver - describes a ULPI PHY driver
+ * @id_table: array of device identifiers supported by this driver
+ * @probe: binds this driver to ULPI device
+ * @remove: unbinds this driver from ULPI device
+ * @driver: the name and owner members must be initialized by the drivers
+ */
+struct ulpi_driver {
+	const struct ulpi_device_id *id_table;
+	int (*probe)(struct ulpi *ulpi);
+	void (*remove)(struct ulpi *ulpi);
+	struct device_driver driver;
+};
+
+#define to_ulpi_driver(d) container_of(d, struct ulpi_driver, driver)
+
+int ulpi_register_driver(struct ulpi_driver *drv);
+void ulpi_unregister_driver(struct ulpi_driver *drv);
+
+#define module_ulpi_driver(__ulpi_driver) \
+	module_driver(__ulpi_driver, ulpi_register_driver, \
+		      ulpi_unregister_driver)
+
+int ulpi_read(struct ulpi *ulpi, u8 addr);
+int ulpi_write(struct ulpi *ulpi, u8 addr, u8 val);
+
+extern struct device_type ulpi_device_type;
+
+#endif /* __LINUX_ULPI_DRIVER_H */
diff --git a/include/linux/ulpi/interface.h b/include/linux/ulpi/interface.h
new file mode 100644
index 0000000..08753f7
--- /dev/null
+++ b/include/linux/ulpi/interface.h
@@ -0,0 +1,23 @@
+#ifndef __LINUX_ULPI_INTERFACE_H
+#define __LINUX_ULPI_INTERFACE_H
+
+#include <linux/types.h>
+
+struct ulpi;
+
+/**
+ * struct ulpi_ops - ULPI register access
+ * @dev: the interface provider
+ * @read: read opearation for ULPI register access
+ * @write: write opearation for ULPI register access
+ */
+struct ulpi_ops {
+	struct device *dev;
+	int (*read)(struct ulpi_ops *ops, u8 addr);
+	int (*write)(struct ulpi_ops *ops, u8 addr, u8 val);
+};
+
+struct ulpi *ulpi_register_interface(struct device *, struct ulpi_ops *);
+void ulpi_unregister_interface(struct ulpi *);
+
+#endif /* __LINUX_ULPI_INTERFACE_H */
diff --git a/include/linux/ulpi/regs.h b/include/linux/ulpi/regs.h
new file mode 100644
index 0000000..b5b8b88
--- /dev/null
+++ b/include/linux/ulpi/regs.h
@@ -0,0 +1,130 @@
+#ifndef __LINUX_ULPI_REGS_H
+#define __LINUX_ULPI_REGS_H
+
+/*
+ * Macros for Set and Clear
+ * See ULPI 1.1 specification to find the registers with Set and Clear offsets
+ */
+#define ULPI_SET(a)				(a + 1)
+#define ULPI_CLR(a)				(a + 2)
+
+/*
+ * Register Map
+ */
+#define ULPI_VENDOR_ID_LOW			0x00
+#define ULPI_VENDOR_ID_HIGH			0x01
+#define ULPI_PRODUCT_ID_LOW			0x02
+#define ULPI_PRODUCT_ID_HIGH			0x03
+#define ULPI_FUNC_CTRL				0x04
+#define ULPI_IFC_CTRL				0x07
+#define ULPI_OTG_CTRL				0x0a
+#define ULPI_USB_INT_EN_RISE			0x0d
+#define ULPI_USB_INT_EN_FALL			0x10
+#define ULPI_USB_INT_STS			0x13
+#define ULPI_USB_INT_LATCH			0x14
+#define ULPI_DEBUG				0x15
+#define ULPI_SCRATCH				0x16
+/* Optional Carkit Registers */
+#define ULPI_CARKIT_CTRL			0x19
+#define ULPI_CARKIT_INT_DELAY			0x1c
+#define ULPI_CARKIT_INT_EN			0x1d
+#define ULPI_CARKIT_INT_STS			0x20
+#define ULPI_CARKIT_INT_LATCH			0x21
+#define ULPI_CARKIT_PLS_CTRL			0x22
+/* Other Optional Registers */
+#define ULPI_TX_POS_WIDTH			0x25
+#define ULPI_TX_NEG_WIDTH			0x26
+#define ULPI_POLARITY_RECOVERY			0x27
+/* Access Extended Register Set */
+#define ULPI_ACCESS_EXTENDED			0x2f
+/* Vendor Specific */
+#define ULPI_VENDOR_SPECIFIC			0x30
+/* Extended Registers */
+#define ULPI_EXT_VENDOR_SPECIFIC		0x80
+
+/*
+ * Register Bits
+ */
+
+/* Function Control */
+#define ULPI_FUNC_CTRL_XCVRSEL			BIT(0)
+#define  ULPI_FUNC_CTRL_XCVRSEL_MASK		0x3
+#define  ULPI_FUNC_CTRL_HIGH_SPEED		0x0
+#define  ULPI_FUNC_CTRL_FULL_SPEED		0x1
+#define  ULPI_FUNC_CTRL_LOW_SPEED		0x2
+#define  ULPI_FUNC_CTRL_FS4LS			0x3
+#define ULPI_FUNC_CTRL_TERMSELECT		BIT(2)
+#define ULPI_FUNC_CTRL_OPMODE			BIT(3)
+#define  ULPI_FUNC_CTRL_OPMODE_MASK		(0x3 << 3)
+#define  ULPI_FUNC_CTRL_OPMODE_NORMAL		(0x0 << 3)
+#define  ULPI_FUNC_CTRL_OPMODE_NONDRIVING	(0x1 << 3)
+#define  ULPI_FUNC_CTRL_OPMODE_DISABLE_NRZI	(0x2 << 3)
+#define  ULPI_FUNC_CTRL_OPMODE_NOSYNC_NOEOP	(0x3 << 3)
+#define ULPI_FUNC_CTRL_RESET			BIT(5)
+#define ULPI_FUNC_CTRL_SUSPENDM			BIT(6)
+
+/* Interface Control */
+#define ULPI_IFC_CTRL_6_PIN_SERIAL_MODE		BIT(0)
+#define ULPI_IFC_CTRL_3_PIN_SERIAL_MODE		BIT(1)
+#define ULPI_IFC_CTRL_CARKITMODE		BIT(2)
+#define ULPI_IFC_CTRL_CLOCKSUSPENDM		BIT(3)
+#define ULPI_IFC_CTRL_AUTORESUME		BIT(4)
+#define ULPI_IFC_CTRL_EXTERNAL_VBUS		BIT(5)
+#define ULPI_IFC_CTRL_PASSTHRU			BIT(6)
+#define ULPI_IFC_CTRL_PROTECT_IFC_DISABLE	BIT(7)
+
+/* OTG Control */
+#define ULPI_OTG_CTRL_ID_PULLUP			BIT(0)
+#define ULPI_OTG_CTRL_DP_PULLDOWN		BIT(1)
+#define ULPI_OTG_CTRL_DM_PULLDOWN		BIT(2)
+#define ULPI_OTG_CTRL_DISCHRGVBUS		BIT(3)
+#define ULPI_OTG_CTRL_CHRGVBUS			BIT(4)
+#define ULPI_OTG_CTRL_DRVVBUS			BIT(5)
+#define ULPI_OTG_CTRL_DRVVBUS_EXT		BIT(6)
+#define ULPI_OTG_CTRL_EXTVBUSIND		BIT(7)
+
+/* USB Interrupt Enable Rising,
+ * USB Interrupt Enable Falling,
+ * USB Interrupt Status and
+ * USB Interrupt Latch
+ */
+#define ULPI_INT_HOST_DISCONNECT		BIT(0)
+#define ULPI_INT_VBUS_VALID			BIT(1)
+#define ULPI_INT_SESS_VALID			BIT(2)
+#define ULPI_INT_SESS_END			BIT(3)
+#define ULPI_INT_IDGRD				BIT(4)
+
+/* Debug */
+#define ULPI_DEBUG_LINESTATE0			BIT(0)
+#define ULPI_DEBUG_LINESTATE1			BIT(1)
+
+/* Carkit Control */
+#define ULPI_CARKIT_CTRL_CARKITPWR		BIT(0)
+#define ULPI_CARKIT_CTRL_IDGNDDRV		BIT(1)
+#define ULPI_CARKIT_CTRL_TXDEN			BIT(2)
+#define ULPI_CARKIT_CTRL_RXDEN			BIT(3)
+#define ULPI_CARKIT_CTRL_SPKLEFTEN		BIT(4)
+#define ULPI_CARKIT_CTRL_SPKRIGHTEN		BIT(5)
+#define ULPI_CARKIT_CTRL_MICEN			BIT(6)
+
+/* Carkit Interrupt Enable */
+#define ULPI_CARKIT_INT_EN_IDFLOAT_RISE		BIT(0)
+#define ULPI_CARKIT_INT_EN_IDFLOAT_FALL		BIT(1)
+#define ULPI_CARKIT_INT_EN_CARINTDET		BIT(2)
+#define ULPI_CARKIT_INT_EN_DP_RISE		BIT(3)
+#define ULPI_CARKIT_INT_EN_DP_FALL		BIT(4)
+
+/* Carkit Interrupt Status and
+ * Carkit Interrupt Latch
+ */
+#define ULPI_CARKIT_INT_IDFLOAT			BIT(0)
+#define ULPI_CARKIT_INT_CARINTDET		BIT(1)
+#define ULPI_CARKIT_INT_DP			BIT(2)
+
+/* Carkit Pulse Control*/
+#define ULPI_CARKIT_PLS_CTRL_TXPLSEN		BIT(0)
+#define ULPI_CARKIT_PLS_CTRL_RXPLSEN		BIT(1)
+#define ULPI_CARKIT_PLS_CTRL_SPKRLEFT_BIASEN	BIT(2)
+#define ULPI_CARKIT_PLS_CTRL_SPKRRIGHT_BIASEN	BIT(3)
+
+#endif /* __LINUX_ULPI_REGS_H */
diff --git a/include/linux/usb/ulpi.h b/include/linux/usb/ulpi.h
index 5c295c2..5f07407 100644
--- a/include/linux/usb/ulpi.h
+++ b/include/linux/usb/ulpi.h
@@ -12,6 +12,8 @@
 #define __LINUX_USB_ULPI_H
 
 #include <linux/usb/otg.h>
+#include <linux/ulpi/regs.h>
+
 /*-------------------------------------------------------------------------*/
 
 /*
@@ -49,138 +51,6 @@
 
 /*-------------------------------------------------------------------------*/
 
-/*
- * Macros for Set and Clear
- * See ULPI 1.1 specification to find the registers with Set and Clear offsets
- */
-#define ULPI_SET(a)				(a + 1)
-#define ULPI_CLR(a)				(a + 2)
-
-/*-------------------------------------------------------------------------*/
-
-/*
- * Register Map
- */
-#define ULPI_VENDOR_ID_LOW			0x00
-#define ULPI_VENDOR_ID_HIGH			0x01
-#define ULPI_PRODUCT_ID_LOW			0x02
-#define ULPI_PRODUCT_ID_HIGH			0x03
-#define ULPI_FUNC_CTRL				0x04
-#define ULPI_IFC_CTRL				0x07
-#define ULPI_OTG_CTRL				0x0a
-#define ULPI_USB_INT_EN_RISE			0x0d
-#define ULPI_USB_INT_EN_FALL			0x10
-#define ULPI_USB_INT_STS			0x13
-#define ULPI_USB_INT_LATCH			0x14
-#define ULPI_DEBUG				0x15
-#define ULPI_SCRATCH				0x16
-/* Optional Carkit Registers */
-#define ULPI_CARCIT_CTRL			0x19
-#define ULPI_CARCIT_INT_DELAY			0x1c
-#define ULPI_CARCIT_INT_EN			0x1d
-#define ULPI_CARCIT_INT_STS			0x20
-#define ULPI_CARCIT_INT_LATCH			0x21
-#define ULPI_CARCIT_PLS_CTRL			0x22
-/* Other Optional Registers */
-#define ULPI_TX_POS_WIDTH			0x25
-#define ULPI_TX_NEG_WIDTH			0x26
-#define ULPI_POLARITY_RECOVERY			0x27
-/* Access Extended Register Set */
-#define ULPI_ACCESS_EXTENDED			0x2f
-/* Vendor Specific */
-#define ULPI_VENDOR_SPECIFIC			0x30
-/* Extended Registers */
-#define ULPI_EXT_VENDOR_SPECIFIC		0x80
-
-/*-------------------------------------------------------------------------*/
-
-/*
- * Register Bits
- */
-
-/* Function Control */
-#define ULPI_FUNC_CTRL_XCVRSEL			(1 << 0)
-#define  ULPI_FUNC_CTRL_XCVRSEL_MASK		(3 << 0)
-#define  ULPI_FUNC_CTRL_HIGH_SPEED		(0 << 0)
-#define  ULPI_FUNC_CTRL_FULL_SPEED		(1 << 0)
-#define  ULPI_FUNC_CTRL_LOW_SPEED		(2 << 0)
-#define  ULPI_FUNC_CTRL_FS4LS			(3 << 0)
-#define ULPI_FUNC_CTRL_TERMSELECT		(1 << 2)
-#define ULPI_FUNC_CTRL_OPMODE			(1 << 3)
-#define  ULPI_FUNC_CTRL_OPMODE_MASK		(3 << 3)
-#define  ULPI_FUNC_CTRL_OPMODE_NORMAL		(0 << 3)
-#define  ULPI_FUNC_CTRL_OPMODE_NONDRIVING	(1 << 3)
-#define  ULPI_FUNC_CTRL_OPMODE_DISABLE_NRZI	(2 << 3)
-#define  ULPI_FUNC_CTRL_OPMODE_NOSYNC_NOEOP	(3 << 3)
-#define ULPI_FUNC_CTRL_RESET			(1 << 5)
-#define ULPI_FUNC_CTRL_SUSPENDM			(1 << 6)
-
-/* Interface Control */
-#define ULPI_IFC_CTRL_6_PIN_SERIAL_MODE		(1 << 0)
-#define ULPI_IFC_CTRL_3_PIN_SERIAL_MODE		(1 << 1)
-#define ULPI_IFC_CTRL_CARKITMODE		(1 << 2)
-#define ULPI_IFC_CTRL_CLOCKSUSPENDM		(1 << 3)
-#define ULPI_IFC_CTRL_AUTORESUME		(1 << 4)
-#define ULPI_IFC_CTRL_EXTERNAL_VBUS		(1 << 5)
-#define ULPI_IFC_CTRL_PASSTHRU			(1 << 6)
-#define ULPI_IFC_CTRL_PROTECT_IFC_DISABLE	(1 << 7)
-
-/* OTG Control */
-#define ULPI_OTG_CTRL_ID_PULLUP			(1 << 0)
-#define ULPI_OTG_CTRL_DP_PULLDOWN		(1 << 1)
-#define ULPI_OTG_CTRL_DM_PULLDOWN		(1 << 2)
-#define ULPI_OTG_CTRL_DISCHRGVBUS		(1 << 3)
-#define ULPI_OTG_CTRL_CHRGVBUS			(1 << 4)
-#define ULPI_OTG_CTRL_DRVVBUS			(1 << 5)
-#define ULPI_OTG_CTRL_DRVVBUS_EXT		(1 << 6)
-#define ULPI_OTG_CTRL_EXTVBUSIND		(1 << 7)
-
-/* USB Interrupt Enable Rising,
- * USB Interrupt Enable Falling,
- * USB Interrupt Status and
- * USB Interrupt Latch
- */
-#define ULPI_INT_HOST_DISCONNECT		(1 << 0)
-#define ULPI_INT_VBUS_VALID			(1 << 1)
-#define ULPI_INT_SESS_VALID			(1 << 2)
-#define ULPI_INT_SESS_END			(1 << 3)
-#define ULPI_INT_IDGRD				(1 << 4)
-
-/* Debug */
-#define ULPI_DEBUG_LINESTATE0			(1 << 0)
-#define ULPI_DEBUG_LINESTATE1			(1 << 1)
-
-/* Carkit Control */
-#define ULPI_CARKIT_CTRL_CARKITPWR		(1 << 0)
-#define ULPI_CARKIT_CTRL_IDGNDDRV		(1 << 1)
-#define ULPI_CARKIT_CTRL_TXDEN			(1 << 2)
-#define ULPI_CARKIT_CTRL_RXDEN			(1 << 3)
-#define ULPI_CARKIT_CTRL_SPKLEFTEN		(1 << 4)
-#define ULPI_CARKIT_CTRL_SPKRIGHTEN		(1 << 5)
-#define ULPI_CARKIT_CTRL_MICEN			(1 << 6)
-
-/* Carkit Interrupt Enable */
-#define ULPI_CARKIT_INT_EN_IDFLOAT_RISE		(1 << 0)
-#define ULPI_CARKIT_INT_EN_IDFLOAT_FALL		(1 << 1)
-#define ULPI_CARKIT_INT_EN_CARINTDET		(1 << 2)
-#define ULPI_CARKIT_INT_EN_DP_RISE		(1 << 3)
-#define ULPI_CARKIT_INT_EN_DP_FALL		(1 << 4)
-
-/* Carkit Interrupt Status and
- * Carkit Interrupt Latch
- */
-#define ULPI_CARKIT_INT_IDFLOAT			(1 << 0)
-#define ULPI_CARKIT_INT_CARINTDET		(1 << 1)
-#define ULPI_CARKIT_INT_DP			(1 << 2)
-
-/* Carkit Pulse Control*/
-#define ULPI_CARKIT_PLS_CTRL_TXPLSEN		(1 << 0)
-#define ULPI_CARKIT_PLS_CTRL_RXPLSEN		(1 << 1)
-#define ULPI_CARKIT_PLS_CTRL_SPKRLEFT_BIASEN	(1 << 2)
-#define ULPI_CARKIT_PLS_CTRL_SPKRRIGHT_BIASEN	(1 << 3)
-
-/*-------------------------------------------------------------------------*/
-
 #if IS_ENABLED(CONFIG_USB_ULPI)
 struct usb_phy *otg_ulpi_create(struct usb_phy_io_ops *ops,
 					unsigned int flags);
diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index f282516..2b55464 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -186,5 +186,9 @@ int main(void)
 	DEVID_FIELD(rio_device_id, asm_did);
 	DEVID_FIELD(rio_device_id, asm_vid);
 
+	DEVID(ulpi_device_id);
+	DEVID_FIELD(ulpi_device_id, vendor);
+	DEVID_FIELD(ulpi_device_id, product);
+
 	return 0;
 }
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index e614ef6..753cb08 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1176,6 +1176,19 @@ static int do_rio_entry(const char *filename,
 }
 ADD_TO_DEVTABLE("rapidio", rio_device_id, do_rio_entry);
 
+/* Looks like: mei:S */
+static int do_ulpi_entry(const char *filename, void *symval,
+			 char *alias)
+{
+	DEF_FIELD(symval, ulpi_device_id, vendor);
+	DEF_FIELD(symval, ulpi_device_id, product);
+
+	sprintf(alias, "ulpi:v%04xp%04x", vendor, product);
+
+	return 1;
+}
+ADD_TO_DEVTABLE("ulpi", ulpi_device_id, do_ulpi_entry);
+
 /* Does namelen bytes of name exactly match the symbol? */
 static bool sym_is(const char *name, unsigned namelen, const char *symbol)
 {
-- 
2.1.4

[PATCH 2/8] usb: dwc3: USB2 PHY register access bits

[Heikki Krogerus]

Definitions for Global USB2 PHY Vendor Control Register
bits. We will need them to access ULPI PHY registers later.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/dwc3/core.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 0842aa8..be1c20e 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -174,6 +174,14 @@
 #define DWC3_GUSB2PHYCFG_PHYSOFTRST	(1 << 31)
 #define DWC3_GUSB2PHYCFG_SUSPHY		(1 << 6)
 
+/* Global USB2 PHY Vendor Control Register */
+#define DWC3_GUSB2PHYACC_NEWREGREQ	(1 << 25)
+#define DWC3_GUSB2PHYACC_BUSY		(1 << 23)
+#define DWC3_GUSB2PHYACC_WRITE		(1 << 22)
+#define DWC3_GUSB2PHYACC_ADDR(n)	(n << 16)
+#define DWC3_GUSB2PHYACC_EXTEND_ADDR(n)	(n << 8)
+#define DWC3_GUSB2PHYACC_DATA(n)	(n & 0xff)
+
 /* Global USB3 PIPE Control Register */
 #define DWC3_GUSB3PIPECTL_PHYSOFTRST	(1 << 31)
 #define DWC3_GUSB3PIPECTL_U2SSINP3OK	(1 << 29)
-- 
2.1.4

[PATCH 3/8] usb: dwc3: store driver data earlier

[Heikki Krogerus]

We need to store it before phys are handled, so we can later
use it in ULPI interface support code.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/dwc3/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 25ddc39..b06e88b 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -876,12 +876,13 @@ static int dwc3_probe(struct platform_device *pdev)
 	dwc->hird_threshold = hird_threshold
 		| (dwc->is_utmi_l1_suspend << 4);
 
+	platform_set_drvdata(pdev, dwc);
+
 	ret = dwc3_core_get_phy(dwc);
 	if (ret)
 		return ret;
 
 	spin_lock_init(&dwc->lock);
-	platform_set_drvdata(pdev, dwc);
 
 	if (!dev->dma_mask) {
 		dev->dma_mask = dev->parent->dma_mask;
-- 
2.1.4

[PATCH 4/8] usb: dwc3: cache hwparams earlier

[Heikki Krogerus]

So they are available when ULPI interface support is added.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/dwc3/core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index b06e88b..a8c9062 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -877,6 +877,7 @@ static int dwc3_probe(struct platform_device *pdev)
 		| (dwc->is_utmi_l1_suspend << 4);
 
 	platform_set_drvdata(pdev, dwc);
+	dwc3_cache_hwparams(dwc);
 
 	ret = dwc3_core_get_phy(dwc);
 	if (ret)
@@ -894,8 +895,6 @@ static int dwc3_probe(struct platform_device *pdev)
 	pm_runtime_get_sync(dev);
 	pm_runtime_forbid(dev);
 
-	dwc3_cache_hwparams(dwc);
-
 	ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
 	if (ret) {
 		dev_err(dwc->dev, "failed to allocate event buffers\n");
-- 
2.1.4

[PATCH 5/8] usb: dwc3: ULPI or UTMI+ select

[Heikki Krogerus]

Make selection between ULPI and UTMI+ interfaces possible by
providing definition for the bit in Global USB2 PHY
Configuration Register that controls it.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/dwc3/core.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index be1c20e..9b6f4c7 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -173,6 +173,7 @@
 /* Global USB2 PHY Configuration Register */
 #define DWC3_GUSB2PHYCFG_PHYSOFTRST	(1 << 31)
 #define DWC3_GUSB2PHYCFG_SUSPHY		(1 << 6)
+#define DWC3_GUSB2PHYCFG_ULPI_UTMI	(1 << 4)
 
 /* Global USB2 PHY Vendor Control Register */
 #define DWC3_GUSB2PHYACC_NEWREGREQ	(1 << 25)
-- 
2.1.4

[PATCH 6/8] usb: dwc3: add ULPI interface support

[Heikki Krogerus]

Registers DWC3's ULPI interface with the ULPI bus when it's
available.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/dwc3/Kconfig  |   7 +++
 drivers/usb/dwc3/Makefile |   4 ++
 drivers/usb/dwc3/core.c   |   6 +++
 drivers/usb/dwc3/core.h   |  13 ++++++
 drivers/usb/dwc3/ulpi.c   | 112 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 142 insertions(+)
 create mode 100644 drivers/usb/dwc3/ulpi.c

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 58b5b2c..cda82ad 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -11,6 +11,13 @@ config USB_DWC3
 
 if USB_DWC3
 
+config USB_DWC3_ULPI
+	bool "Register ULPI PHY Interface"
+	depends on USB_ULPI_BUS=y
+	help
+	  Select this if you have ULPI type PHY attached to your DWC3
+	  controller.
+
 choice
 	bool "DWC3 Mode Selection"
 	default USB_DWC3_DUAL_ROLE if (USB && USB_GADGET)
diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index bb34fbc..2fc44e0 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -16,6 +16,10 @@ ifneq ($(filter y,$(CONFIG_USB_DWC3_GADGET) $(CONFIG_USB_DWC3_DUAL_ROLE)),)
 	dwc3-y				+= gadget.o ep0.o
 endif
 
+ifneq ($(CONFIG_USB_DWC3_ULPI),)
+	dwc3-y				+= ulpi.o
+endif
+
 ifneq ($(CONFIG_DEBUG_FS),)
 	dwc3-y				+= debugfs.o
 endif
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index a8c9062..66cbf38 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, dwc);
 	dwc3_cache_hwparams(dwc);
 
+	ret = dwc3_ulpi_init(dwc);
+	if (ret)
+		return ret;
+
 	ret = dwc3_core_get_phy(dwc);
 	if (ret)
 		return ret;
@@ -965,6 +969,7 @@ err1:
 
 err0:
 	dwc3_free_event_buffers(dwc);
+	dwc3_ulpi_exit(dwc);
 
 	return ret;
 }
@@ -984,6 +989,7 @@ static int dwc3_remove(struct platform_device *pdev)
 	phy_power_off(dwc->usb3_generic_phy);
 
 	dwc3_core_exit(dwc);
+	dwc3_ulpi_exit(dwc);
 
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 9b6f4c7..ad6b371 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -30,6 +30,7 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/otg.h>
+#include <linux/ulpi/interface.h>
 
 #include <linux/phy/phy.h>
 
@@ -748,6 +749,8 @@ struct dwc3 {
 	struct phy		*usb2_generic_phy;
 	struct phy		*usb3_generic_phy;
 
+	struct ulpi		*ulpi;
+
 	void __iomem		*regs;
 	size_t			regs_size;
 
@@ -1044,4 +1047,14 @@ static inline int dwc3_gadget_resume(struct dwc3 *dwc)
 }
 #endif /* !IS_ENABLED(CONFIG_USB_DWC3_HOST) */
 
+#if IS_ENABLED(CONFIG_USB_DWC3_ULPI)
+int dwc3_ulpi_init(struct dwc3 *dwc);
+void dwc3_ulpi_exit(struct dwc3 *dwc);
+#else
+static inline int dwc3_ulpi_init(struct dwc3 *dwc)
+{ return 0; }
+static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
+{ }
+#endif
+
 #endif /* __DRIVERS_USB_DWC3_CORE_H */
diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
new file mode 100644
index 0000000..8806981
--- /dev/null
+++ b/drivers/usb/dwc3/ulpi.c
@@ -0,0 +1,112 @@
+/**
+ * ulpi.c - DesignWare USB3 Controller's ULPI PHY interface
+ *
+ * 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/ulpi/regs.h>
+
+#include "core.h"
+#include "io.h"
+
+#define DWC3_ULPI_ADDR(a) \
+		((a >= ULPI_EXT_VENDOR_SPECIFIC) ? \
+		DWC3_GUSB2PHYACC_ADDR(ULPI_ACCESS_EXTENDED) | \
+		DWC3_GUSB2PHYACC_EXTEND_ADDR(a) : DWC3_GUSB2PHYACC_ADDR(a))
+
+static int dwc3_ulpi_busyloop(struct dwc3 *dwc)
+{
+	unsigned count = 1000;
+	u32 reg;
+
+	while (count--) {
+		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYACC(0));
+		if (!(reg & DWC3_GUSB2PHYACC_BUSY))
+			return 0;
+		cpu_relax();
+	}
+
+	return -ETIMEDOUT;
+}
+
+static int dwc3_ulpi_read(struct ulpi_ops *ops, u8 addr)
+{
+	struct dwc3 *dwc = dev_get_drvdata(ops->dev);
+	u32 reg;
+	int ret;
+
+	reg = DWC3_GUSB2PHYACC_NEWREGREQ | DWC3_ULPI_ADDR(addr);
+	dwc3_writel(dwc->regs, DWC3_GUSB2PHYACC(0), reg);
+
+	ret = dwc3_ulpi_busyloop(dwc);
+	if (ret)
+		return ret;
+
+	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYACC(0));
+
+	return DWC3_GUSB2PHYACC_DATA(reg);
+}
+
+static int dwc3_ulpi_write(struct ulpi_ops *ops, u8 addr, u8 val)
+{
+	struct dwc3 *dwc = dev_get_drvdata(ops->dev);
+	u32 reg;
+	int ret;
+
+	reg = DWC3_GUSB2PHYACC_NEWREGREQ | DWC3_ULPI_ADDR(addr);
+	reg |= DWC3_GUSB2PHYACC_WRITE | val;
+	dwc3_writel(dwc->regs, DWC3_GUSB2PHYACC(0), reg);
+
+	ret = dwc3_ulpi_busyloop(dwc);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static struct ulpi_ops dwc3_ulpi = {
+	.read = dwc3_ulpi_read,
+	.write = dwc3_ulpi_write,
+};
+
+int dwc3_ulpi_init(struct dwc3 *dwc)
+{
+	u32 reg;
+
+	/* First check USB2 PHY interface type */
+	switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) {
+	case DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI:
+		/* Select ULPI Interface */
+		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
+		reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI;
+		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+		/* FALLTHROUGH */
+	case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI:
+		break;
+	default:
+		return 0;
+	}
+
+	/* Register the interface */
+	dwc->ulpi = ulpi_register_interface(dwc->dev, &dwc3_ulpi);
+	if (IS_ERR(dwc->ulpi)) {
+		dev_err(dwc->dev, "failed to register ULPI interface");
+		return PTR_ERR(dwc->ulpi);
+	}
+
+	return 0;
+}
+
+void dwc3_ulpi_exit(struct dwc3 *dwc)
+{
+	if (dwc->ulpi) {
+		ulpi_unregister_interface(dwc->ulpi);
+		dwc->ulpi = NULL;
+	}
+}
-- 
2.1.4

[PATCH 7/8] phy: helpers for USB ULPI PHY registering

[Heikki Krogerus]

ULPI PHYs need to be bound to their controllers with a
lookup. This adds helpers that the ULPI drivers can use to
do both, the registration of the PHY and the lookup, at the
same time.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/ulpi_phy.h | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 drivers/phy/ulpi_phy.h

diff --git a/drivers/phy/ulpi_phy.h b/drivers/phy/ulpi_phy.h
new file mode 100644
index 0000000..ac49fb6
--- /dev/null
+++ b/drivers/phy/ulpi_phy.h
@@ -0,0 +1,31 @@
+#include <linux/phy/phy.h>
+
+/**
+ * Helper that registers PHY for a ULPI device and adds a lookup for binding it
+ * and it's controller, which is always the parent.
+ */
+static inline struct phy
+*ulpi_phy_create(struct ulpi *ulpi, struct phy_ops *ops)
+{
+	struct phy *phy;
+	int ret;
+
+	phy = phy_create(&ulpi->dev, NULL, ops);
+	if (IS_ERR(phy))
+		return phy;
+
+	ret = phy_create_lookup(phy, "usb2-phy", dev_name(ulpi->dev.parent));
+	if (ret) {
+		phy_destroy(phy);
+		return ERR_PTR(ret);
+	}
+
+	return phy;
+}
+
+/* Remove a PHY that was created with ulpi_phy_create() and it's lookup. */
+static inline void ulpi_phy_destroy(struct ulpi *ulpi, struct phy *phy)
+{
+	phy_remove_lookup(phy, "usb2-phy", dev_name(ulpi->dev.parent));
+	phy_destroy(phy);
+}
-- 
2.1.4

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

[Heikki Krogerus]

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,
+	.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;
+	}
+
+	/* Store initial eye diagram optimisation value */
+	ret = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
+	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;
+}
+
+static void tusb1210_remove(struct ulpi *ulpi)
+{
+	struct tusb1210 *tusb = ulpi_get_drvdata(ulpi);
+
+	ulpi_phy_destroy(ulpi, tusb->phy);
+}
+
+#define TI_VENDOR_ID 0x0451
+
+static const struct ulpi_device_id tusb1210_ulpi_id[] = {
+	{ TI_VENDOR_ID, 0x1508, },
+	{ },
+};
+MODULE_DEVICE_TABLE(ulpi, tusb1210_ulpi_id);
+
+static struct ulpi_driver tusb1210_driver = {
+	.id_table = tusb1210_ulpi_id,
+	.probe = tusb1210_probe,
+	.remove = tusb1210_remove,
+	.driver = {
+		.name = "tusb1210",
+		.owner = THIS_MODULE,
+	},
+};
+
+module_ulpi_driver(tusb1210_driver);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("TUSB1210 ULPI PHY driver");
-- 
2.1.4

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[Felipe Balbi]

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

Hi,

On Fri, Jan 23, 2015 at 05:12:56PM +0200, Heikki Krogerus wrote:
> +int dwc3_ulpi_init(struct dwc3 *dwc)
> +{
> +	u32 reg;
> +
> +	/* First check USB2 PHY interface type */
> +	switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) {
> +	case DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI:
> +		/* Select ULPI Interface */
> +		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> +		reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI;
> +		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> +		/* FALLTHROUGH */
> +	case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI:
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	/* Register the interface */
> +	dwc->ulpi = ulpi_register_interface(dwc->dev, &dwc3_ulpi);
> +	if (IS_ERR(dwc->ulpi)) {

so, this will only build and link if DWC3_ULPI is enabled, in case of
error you return early...

> +		dev_err(dwc->dev, "failed to register ULPI interface");
> +		return PTR_ERR(dwc->ulpi);
> +	}
> +
> +	return 0;
> +}
> +
> +void dwc3_ulpi_exit(struct dwc3 *dwc)
> +{
> +	if (dwc->ulpi) {

... looks like this branch is unnecessary.

-- 
balbi

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

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

[David Cohen]

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

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(tusb->phy, tusb);
> +	ulpi_set_drvdata(ulpi, tusb);
> +	return 0;
> +}
> +
> +static void tusb1210_remove(struct ulpi *ulpi)
> +{
> +	struct tusb1210 *tusb = ulpi_get_drvdata(ulpi);
> +
> +	ulpi_phy_destroy(ulpi, tusb->phy);
> +}
> +
> +#define TI_VENDOR_ID 0x0451
> +
> +static const struct ulpi_device_id tusb1210_ulpi_id[] = {
> +	{ TI_VENDOR_ID, 0x1508, },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(ulpi, tusb1210_ulpi_id);
> +
> +static struct ulpi_driver tusb1210_driver = {
> +	.id_table = tusb1210_ulpi_id,
> +	.probe = tusb1210_probe,
> +	.remove = tusb1210_remove,
> +	.driver = {
> +		.name = "tusb1210",
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +module_ulpi_driver(tusb1210_driver);
> +
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("TUSB1210 ULPI PHY driver");
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[Heikki Krogerus]

On Fri, Jan 23, 2015 at 10:24:43AM -0600, Felipe Balbi wrote:
> On Fri, Jan 23, 2015 at 05:12:56PM +0200, Heikki Krogerus wrote:
> > +int dwc3_ulpi_init(struct dwc3 *dwc)
> > +{
> > +	u32 reg;
> > +
> > +	/* First check USB2 PHY interface type */
> > +	switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) {
> > +	case DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI:
> > +		/* Select ULPI Interface */
> > +		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> > +		reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI;
> > +		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> > +		/* FALLTHROUGH */
> > +	case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI:
> > +		break;
> > +	default:
> > +		return 0;
> > +	}
> > +
> > +	/* Register the interface */
> > +	dwc->ulpi = ulpi_register_interface(dwc->dev, &dwc3_ulpi);
> > +	if (IS_ERR(dwc->ulpi)) {
> 
> so, this will only build and link if DWC3_ULPI is enabled, in case of
> error you return early...
> 
> > +		dev_err(dwc->dev, "failed to register ULPI interface");
> > +		return PTR_ERR(dwc->ulpi);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void dwc3_ulpi_exit(struct dwc3 *dwc)
> > +{
> > +	if (dwc->ulpi) {
> 
> ... looks like this branch is unnecessary.

We can't do that, or distros that select DWC3_ULPI option can only use
dwc3 with hardware that really has ULPI PHY. So I guess we'll drop the
DWC3_ULPI option and build the dwc3 ulpi support always if ULPI bus is
enabled. OK?


Thanks,

-- 
heikki

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

[Heikki Krogerus]

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

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

[David Cohen]

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

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[Felipe Balbi]

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

On Mon, Jan 26, 2015 at 01:46:10PM +0200, Heikki Krogerus wrote:
> On Fri, Jan 23, 2015 at 10:24:43AM -0600, Felipe Balbi wrote:
> > On Fri, Jan 23, 2015 at 05:12:56PM +0200, Heikki Krogerus wrote:
> > > +int dwc3_ulpi_init(struct dwc3 *dwc)
> > > +{
> > > +	u32 reg;
> > > +
> > > +	/* First check USB2 PHY interface type */
> > > +	switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) {
> > > +	case DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI:
> > > +		/* Select ULPI Interface */
> > > +		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> > > +		reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI;
> > > +		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> > > +		/* FALLTHROUGH */
> > > +	case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI:
> > > +		break;
> > > +	default:
> > > +		return 0;
> > > +	}
> > > +
> > > +	/* Register the interface */
> > > +	dwc->ulpi = ulpi_register_interface(dwc->dev, &dwc3_ulpi);
> > > +	if (IS_ERR(dwc->ulpi)) {
> > 
> > so, this will only build and link if DWC3_ULPI is enabled, in case of
> > error you return early...
> > 
> > > +		dev_err(dwc->dev, "failed to register ULPI interface");
> > > +		return PTR_ERR(dwc->ulpi);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void dwc3_ulpi_exit(struct dwc3 *dwc)
> > > +{
> > > +	if (dwc->ulpi) {
> > 
> > ... looks like this branch is unnecessary.
> 
> We can't do that, or distros that select DWC3_ULPI option can only use
> dwc3 with hardware that really has ULPI PHY. So I guess we'll drop the
> DWC3_ULPI option and build the dwc3 ulpi support always if ULPI bus is
> enabled. OK?

look at your patch again. In case DWC3_ULPI isn't enabled, this file
won't be linked at all. You're using stubs, so taht's fine.

In case it _is_ enabled, you're breaking out early if you can't register
ulpi interface, meaning you're exitting probe() (which, in fact, seems
wrong as I want to be able to run dwc3 with ULPI enabled on a platform
that was configured with ULPI+UTMI, but uses UTMI PHY).

I still think this patch won't work in all cases.

-- 
balbi

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

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

[Heikki Krogerus]

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

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

[-- Attachment #2: baytrail_quirk.diff --]
[-- Type: text/plain, Size: 1021 bytes --]

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 8d95056..53902ea 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -21,6 +21,7 @@
 #include <linux/slab.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
 
 #include "platform_data.h"
 
@@ -35,6 +36,24 @@
 
 static int dwc3_pci_quirks(struct pci_dev *pdev)
 {
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
+	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
+		struct gpio_desc *gpio;
+
+		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
+		if (!IS_ERR(gpio)) {
+			gpiod_direction_output(gpio, 0);
+			gpiod_set_value_cansleep(gpio, 1);
+			gpiod_put(gpio);
+		}
+		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
+		if (!IS_ERR(gpio)) {
+			gpiod_direction_output(gpio, 0);
+			gpiod_set_value_cansleep(gpio, 1);
+			gpiod_put(gpio);
+		}
+	}
+
 	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
 	    pdev->device == PCI_DEVICE_ID_AMD_NL_USB) {
 		struct dwc3_platform_data pdata;

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[Heikki Krogerus]

> look at your patch again. In case DWC3_ULPI isn't enabled, this file
> won't be linked at all. You're using stubs, so taht's fine.
> 
> In case it _is_ enabled, you're breaking out early if you can't register
> ulpi interface, meaning you're exitting probe() (which, in fact, seems
> wrong as I want to be able to run dwc3 with ULPI enabled on a platform
> that was configured with ULPI+UTMI, but uses UTMI PHY).
> 
> I still think this patch won't work in all cases.

OK. So in case of ULPI+UTMI we'll try the ulpi interface, and if it
fails, fall back to UTMI. Or can we use some other method to determine
to which interface the PHY is really attached to in that case?


Thanks,

-- 
heikki

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

[Heikki Krogerus]

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

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[Felipe Balbi]

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

On Tue, Jan 27, 2015 at 01:09:25PM +0200, Heikki Krogerus wrote:
> > look at your patch again. In case DWC3_ULPI isn't enabled, this file
> > won't be linked at all. You're using stubs, so taht's fine.
> > 
> > In case it _is_ enabled, you're breaking out early if you can't register
> > ulpi interface, meaning you're exitting probe() (which, in fact, seems
> > wrong as I want to be able to run dwc3 with ULPI enabled on a platform
> > that was configured with ULPI+UTMI, but uses UTMI PHY).
> > 
> > I still think this patch won't work in all cases.
> 
> OK. So in case of ULPI+UTMI we'll try the ulpi interface, and if it
> fails, fall back to UTMI. Or can we use some other method to determine
> to which interface the PHY is really attached to in that case?

I think we would need a phy_interface_sel pdata/DT binding for those
cases. It should be optional and probably only needed for a few odd
devices.

-- 
balbi

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

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

[David Cohen]

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

Br, David

> 
> 
> 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 <linux/slab.h>
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
>  
>  #include "platform_data.h"
>  
> @@ -35,6 +36,24 @@
>  
>  static int dwc3_pci_quirks(struct pci_dev *pdev)
>  {
> +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> +	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> +		struct gpio_desc *gpio;
> +
> +		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> +		if (!IS_ERR(gpio)) {
> +			gpiod_direction_output(gpio, 0);
> +			gpiod_set_value_cansleep(gpio, 1);
> +			gpiod_put(gpio);
> +		}
> +		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> +		if (!IS_ERR(gpio)) {
> +			gpiod_direction_output(gpio, 0);
> +			gpiod_set_value_cansleep(gpio, 1);
> +			gpiod_put(gpio);
> +		}
> +	}
> +
>  	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
>  	    pdev->device == PCI_DEVICE_ID_AMD_NL_USB) {
>  		struct dwc3_platform_data pdata;

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

[Heikki Krogerus]

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

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

[David Cohen]

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

> 
> 
> Thanks,
> 
> -- 
> heikki

Re: [PATCH 1/8] usb: add bus type for USB ULPI

[Felipe Balbi]

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

Hi,

On Fri, Jan 23, 2015 at 05:12:51PM +0200, Heikki Krogerus wrote:
> @@ -6,4 +6,5 @@ obj-$(CONFIG_USB_COMMON)	  += usb-common.o
>  usb-common-y			  += common.o
>  usb-common-$(CONFIG_USB_LED_TRIG) += led.o
>  
> -obj-$(CONFIG_USB_OTG_FSM) += usb-otg-fsm.o
> +obj-$(CONFIG_USB_OTG_FSM)	+= usb-otg-fsm.o

a bit of unrelated change here.

> diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
> index cc0ced0..c0fc3b5 100644
> --- a/drivers/usb/core/Kconfig
> +++ b/drivers/usb/core/Kconfig
> @@ -84,3 +84,11 @@ config USB_OTG_FSM
>  	  Implements OTG Finite State Machine as specified in On-The-Go
>  	  and Embedded Host Supplement to the USB Revision 2.0 Specification.
>  
> +config USB_ULPI_BUS
> +	bool "USB ULPI PHY interface support"
> +	depends on USB || USB_GADGET

depends on USB_SUPPORT ?

> +int ulpi_register_driver(struct ulpi_driver *drv);
> +void ulpi_unregister_driver(struct ulpi_driver *drv);
> +
> +#define module_ulpi_driver(__ulpi_driver) \
> +	module_driver(__ulpi_driver, ulpi_register_driver, \
> +		      ulpi_unregister_driver)
> +
> +int ulpi_read(struct ulpi *ulpi, u8 addr);
> +int ulpi_write(struct ulpi *ulpi, u8 addr, u8 val);

no stubs ?

-- 
balbi

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

Re: [PATCH 7/8] phy: helpers for USB ULPI PHY registering

[Felipe Balbi]

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

On Fri, Jan 23, 2015 at 05:12:57PM +0200, Heikki Krogerus wrote:
> ULPI PHYs need to be bound to their controllers with a
> lookup. This adds helpers that the ULPI drivers can use to
> do both, the registration of the PHY and the lookup, at the
> same time.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/phy/ulpi_phy.h | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 drivers/phy/ulpi_phy.h
> 
> diff --git a/drivers/phy/ulpi_phy.h b/drivers/phy/ulpi_phy.h
> new file mode 100644
> index 0000000..ac49fb6
> --- /dev/null
> +++ b/drivers/phy/ulpi_phy.h
> @@ -0,0 +1,31 @@
> +#include <linux/phy/phy.h>
> +
> +/**
> + * Helper that registers PHY for a ULPI device and adds a lookup for binding it
> + * and it's controller, which is always the parent.
> + */
> +static inline struct phy
> +*ulpi_phy_create(struct ulpi *ulpi, struct phy_ops *ops)
> +{
> +	struct phy *phy;
> +	int ret;
> +
> +	phy = phy_create(&ulpi->dev, NULL, ops);
> +	if (IS_ERR(phy))
> +		return phy;
> +
> +	ret = phy_create_lookup(phy, "usb2-phy", dev_name(ulpi->dev.parent));

I wonder if this would break DT-based boot where PHY phandle is
available. Kishon ?

-- 
balbi

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

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

[Felipe Balbi]

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

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

[Heikki Krogerus]

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

> > > > > 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 know about new
> ULPI bus. They rely on platform devices created by pci probe. Your ULPI
> bus proposal is way better to handle that problem and got my support
> since they beginning you showed that to me, but it does not justify
> breaking current working devices. Removing the platform device
> registration for phy with firmwares that rely on that was a mistake and
> any ACPI work related to fix that is unnecessary. These legacy ACPI
> tables gave the phy-related GPIOs to dwc3. Just mark is as legacy
> situation and let the legacy hw's happy. No vendor will change their
> BIOS after market due to non-buggy situation.

Well, I'm really not expecting any BIOS updates any more. I assumed
that was clear. Why else would I have started the whole planning of
the GPIO forwarding. But if it wasn't, then sorry. Now you know.

BYT-CR's USB is not supported in mainline yet unless I'm completely
mistaken, but we have the plan for it. Instead of trying to take any
shortcuts, let's follow that plan.

Because of the need to write to the ULPI registers, I don't think we
should try anything else except to use ULPI bus straight away. We'll
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.

Then we'll pass the GPIO to the PHY driver in the same quirk once we
have the support for it in gpiolib.


Thanks,

-- 
heikki

[-- Attachment #2: baytrail_quirk.diff --]
[-- Type: text/plain, Size: 1021 bytes --]

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 8d95056..53902ea 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -21,6 +21,7 @@
 #include <linux/slab.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
 
 #include "platform_data.h"
 
@@ -35,6 +36,24 @@
 
 static int dwc3_pci_quirks(struct pci_dev *pdev)
 {
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
+	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
+		struct gpio_desc *gpio;
+
+		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
+		if (!IS_ERR(gpio)) {
+			gpiod_direction_output(gpio, 0);
+			gpiod_set_value_cansleep(gpio, 1);
+			gpiod_put(gpio);
+		}
+		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
+		if (!IS_ERR(gpio)) {
+			gpiod_direction_output(gpio, 0);
+			gpiod_set_value_cansleep(gpio, 1);
+			gpiod_put(gpio);
+		}
+	}
+
 	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
 	    pdev->device == PCI_DEVICE_ID_AMD_NL_USB) {
 		struct dwc3_platform_data pdata;

Re: [PATCH 1/8] usb: add bus type for USB ULPI

[Heikki Krogerus]

On Wed, Jan 28, 2015 at 11:02:37PM -0600, Felipe Balbi wrote:
> Hi,
> 
> On Fri, Jan 23, 2015 at 05:12:51PM +0200, Heikki Krogerus wrote:
> > @@ -6,4 +6,5 @@ obj-$(CONFIG_USB_COMMON)	  += usb-common.o
> >  usb-common-y			  += common.o
> >  usb-common-$(CONFIG_USB_LED_TRIG) += led.o
> >  
> > -obj-$(CONFIG_USB_OTG_FSM) += usb-otg-fsm.o
> > +obj-$(CONFIG_USB_OTG_FSM)	+= usb-otg-fsm.o
> 
> a bit of unrelated change here.

OK

> > diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
> > index cc0ced0..c0fc3b5 100644
> > --- a/drivers/usb/core/Kconfig
> > +++ b/drivers/usb/core/Kconfig
> > @@ -84,3 +84,11 @@ config USB_OTG_FSM
> >  	  Implements OTG Finite State Machine as specified in On-The-Go
> >  	  and Embedded Host Supplement to the USB Revision 2.0 Specification.
> >  
> > +config USB_ULPI_BUS
> > +	bool "USB ULPI PHY interface support"
> > +	depends on USB || USB_GADGET
> 
> depends on USB_SUPPORT ?

sure

> > +int ulpi_register_driver(struct ulpi_driver *drv);
> > +void ulpi_unregister_driver(struct ulpi_driver *drv);
> > +
> > +#define module_ulpi_driver(__ulpi_driver) \
> > +	module_driver(__ulpi_driver, ulpi_register_driver, \
> > +		      ulpi_unregister_driver)
> > +
> > +int ulpi_read(struct ulpi *ulpi, u8 addr);
> > +int ulpi_write(struct ulpi *ulpi, u8 addr, u8 val);
> 
> no stubs ?

Is there really need for them? Well, I'll add them.


Thanks,

-- 
heikki

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

[Heikki Krogerus]

> 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

Re: [PATCH 7/8] phy: helpers for USB ULPI PHY registering

[Heikki Krogerus]

On Wed, Jan 28, 2015 at 11:03:55PM -0600, Felipe Balbi wrote:
> On Fri, Jan 23, 2015 at 05:12:57PM +0200, Heikki Krogerus wrote:
> > ULPI PHYs need to be bound to their controllers with a
> > lookup. This adds helpers that the ULPI drivers can use to
> > do both, the registration of the PHY and the lookup, at the
> > same time.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > ---
> >  drivers/phy/ulpi_phy.h | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >  create mode 100644 drivers/phy/ulpi_phy.h
> > 
> > diff --git a/drivers/phy/ulpi_phy.h b/drivers/phy/ulpi_phy.h
> > new file mode 100644
> > index 0000000..ac49fb6
> > --- /dev/null
> > +++ b/drivers/phy/ulpi_phy.h
> > @@ -0,0 +1,31 @@
> > +#include <linux/phy/phy.h>
> > +
> > +/**
> > + * Helper that registers PHY for a ULPI device and adds a lookup for binding it
> > + * and it's controller, which is always the parent.
> > + */
> > +static inline struct phy
> > +*ulpi_phy_create(struct ulpi *ulpi, struct phy_ops *ops)
> > +{
> > +	struct phy *phy;
> > +	int ret;
> > +
> > +	phy = phy_create(&ulpi->dev, NULL, ops);
> > +	if (IS_ERR(phy))
> > +		return phy;
> > +
> > +	ret = phy_create_lookup(phy, "usb2-phy", dev_name(ulpi->dev.parent));
> 
> I wonder if this would break DT-based boot where PHY phandle is
> available. Kishon ?

If there are of_node for the PHY, we will not use this binding at all.

Br,

-- 
heikki

Re: [PATCH 7/8] phy: helpers for USB ULPI PHY registering

[Felipe Balbi]

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

On Thu, Jan 29, 2015 at 04:34:41PM +0200, Heikki Krogerus wrote:
> On Wed, Jan 28, 2015 at 11:03:55PM -0600, Felipe Balbi wrote:
> > On Fri, Jan 23, 2015 at 05:12:57PM +0200, Heikki Krogerus wrote:
> > > ULPI PHYs need to be bound to their controllers with a
> > > lookup. This adds helpers that the ULPI drivers can use to
> > > do both, the registration of the PHY and the lookup, at the
> > > same time.
> > > 
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > > ---
> > >  drivers/phy/ulpi_phy.h | 31 +++++++++++++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >  create mode 100644 drivers/phy/ulpi_phy.h
> > > 
> > > diff --git a/drivers/phy/ulpi_phy.h b/drivers/phy/ulpi_phy.h
> > > new file mode 100644
> > > index 0000000..ac49fb6
> > > --- /dev/null
> > > +++ b/drivers/phy/ulpi_phy.h
> > > @@ -0,0 +1,31 @@
> > > +#include <linux/phy/phy.h>
> > > +
> > > +/**
> > > + * Helper that registers PHY for a ULPI device and adds a lookup for binding it
> > > + * and it's controller, which is always the parent.
> > > + */
> > > +static inline struct phy
> > > +*ulpi_phy_create(struct ulpi *ulpi, struct phy_ops *ops)
> > > +{
> > > +	struct phy *phy;
> > > +	int ret;
> > > +
> > > +	phy = phy_create(&ulpi->dev, NULL, ops);
> > > +	if (IS_ERR(phy))
> > > +		return phy;
> > > +
> > > +	ret = phy_create_lookup(phy, "usb2-phy", dev_name(ulpi->dev.parent));
> > 
> > I wonder if this would break DT-based boot where PHY phandle is
> > available. Kishon ?
> 
> If there are of_node for the PHY, we will not use this binding at all.

where is that taken care of ? ulpi_phy_create() is called
unconditionally.

-- 
balbi

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

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

[Felipe Balbi]

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

Hi

On Thu, Jan 29, 2015 at 04:14:12PM +0200, Heikki Krogerus wrote:
> > > > > > Can you share how tusb1210 is connected on the platform you're using as
> > > > > > test for this patch? I don't think this driver would work reliably with
> > > > > > this device:
> > > > > > http://liliputing.com/2014/11/trekstor-launches-first-android-tablet-based-intels-irda-reference-design.html
> > > > > 
> > > > > The only reason why that board doesn't work is because of very much
> > > > > Baytrail-CR specific problems. These are are two issues, but the first
> > > > 
> > > > That's not BYT-CR specific problems. That's just dwc3 and tusb1210
> > > > interacting as they're expecting to.
> > > > 
> > > > > one is critical for getting it working. Both will be handled, but
> > > > > separately from this set:
> > > > > 
> > > > > 1) The firmware leaves the PHY in reset, forcing us to enable it
> > > > > somehow in OS before accessing ulpi. Unless we can get a firmware fix
> > > > > for that (it's starting to look like it's not going to happen; please
> > > > > correct me if you know something else!), we need to add a quirk for
> > > > > Baytrails (attached), which is probable still OK. But IMO this really
> > > > > should be fixed in the firmware.
> > > > 
> > > > It seems you're expecting the PHY to be fully operational in order to
> > > > probe it. That's wrong assumption. BYT-CR's BIOS is doing nothing wrong
> > > > by leaving PHY on reset state.
> > > 
> > > But it is. If we want to use ULPI as a bus like we do, then the PHY
> > > will be no different then devices attached to many other buses. We
> > > have made firmware fixes like that before. All the devices need to be
> > > in a state, operational enough after bootup, so we can probe them in
> > > OS without the need for hacks where they are separately enabled before
> > > it's possible.
> > 
> > That makes no sense. Not only dwc3 and phy could live as modules (which
> > means they may probe far away from device's boot time) but we have
> > examples of buses not behaving like you said. E.g. I2C needs master to
> > be probed to have bus working and no BIOS needs to make I2C controller
> > functional in order to probe its controller's driver.
> 
> You can't really compare a bus like i2c, which can't enumerate devices
> natively, to ULPI which can.

why not ? The BIOS might not need to use the PHY (or USB) at all, it can
very well decide to never turn it on, right ?

> > > > The real problem is what I stated above.
> > > > With your current logic, you'll get stuck with checking/egg problem: you
> > > > need phy probed to probe dwc3, but need dwc3 probed to power on phy.
> > > > You need a logic to break this circular dependency.
> > > 
> > > The moment we register the ulpi interface with the ulpi bus in
> > > dwc3_probe(), we know dwc3 has it's PHY interface in operational mode
> > > and register access to ULPI PHY is possible. And that is all dwc3
> > > needs to/can do.
> > > 
> > > I don't think you are seeing the whole "ulpi bus" in these patches,
> > > but in any case; Like I said, this problem is purely BYT-CR specific,
> > > which IMO really should be fixed in the firmware.
> > 
> > The proposed design has a flaw that breaks products on market simply
> > because they don't have BIOS (unnecessarily) powering on phy. You're
> > labeling that as BYT-CR specific issue because BYT-CR needs to be PM
> > efficient and then it won't power on hw components in moment they don't
> > need to. FW developers won't like this suggestion and I'd have to agree
> > with them.
> 
> What exactly are we breaking here? The USB on BYT-CR does not work yet
> with the mainline kernel, or does it? To enable it, I already
> suggested the BYT quirk (attached again).

one comment below on this.

> I don't think the other boards we have which use TUSB1210, like the
> BYT-I ones and I think some Merrifield based boards, expect any less
> from PM efficiency then BYT-CR, but we don't need to do any tricks
> with them in order to use ULPI bus. That is what I mean when I say
> this is BYT-CR specific problem.

perhaps because firmware on those other boards are powering up the PHY ?

> I don't agree with PM arguments if it means that we should be ready to
> accept loosing possibility for a generic solution in OS with a single
> device like our PHY. I seriously doubt it would prevent the products
> using these boards of achieving their PM requirements. But this
> conversation is outside our topic.

we're not loosing anything. We're just considering what's the best way
to tackle that ulpi_read() inside probe(). TUSB1210 driver _has_ to cope
with situations where reset_gpio/cs_gpio are in unexpected state. Saying
we will just "fix the firmware", as if that was a simple feat, is
counter-productive.

> > > > > 2) Since the gpio resources are given to the controller device in ACPI
> > > > > tables and there isn't separate device object for the PHY at all, we
> > > > > need to deliver the gpios somehow separately to the phy driver. There
> > > > > is a thread where we are talking about how to do that:
> > > > > https://lkml.org/lkml/2014/12/18/82
> > > > 
> > > > How about just leave the logic the way it is:
> > > > dwc3-pci.c registers platform_device with gpio as resource if the GPIOs
> > > > are provided to dwc3. If not, then dwc3-pci.c will expect phy to have
> > > > its own ACPI id.
> > > 
> > > I think you are now talking about the platform devices for the legacy
> > > USB PHY framework created in dwc3-pci.c, which btw. were removed.
> > > Please note that we are not using platform bus with the ULPI devices,
> > > and those devices are created by the bus driver and not dwc3.
> > 
> > Yes, that the one. Current products' BIOS on market didn't know about new
> > ULPI bus. They rely on platform devices created by pci probe. Your ULPI
> > bus proposal is way better to handle that problem and got my support
> > since they beginning you showed that to me, but it does not justify
> > breaking current working devices. Removing the platform device
> > registration for phy with firmwares that rely on that was a mistake and
> > any ACPI work related to fix that is unnecessary. These legacy ACPI
> > tables gave the phy-related GPIOs to dwc3. Just mark is as legacy
> > situation and let the legacy hw's happy. No vendor will change their
> > BIOS after market due to non-buggy situation.
> 
> Well, I'm really not expecting any BIOS updates any more. I assumed
> that was clear. Why else would I have started the whole planning of
> the GPIO forwarding. But if it wasn't, then sorry. Now you know.
> 
> BYT-CR's USB is not supported in mainline yet unless I'm completely
> mistaken, but we have the plan for it. Instead of trying to take any
> shortcuts, let's follow that plan.
> 
> Because of the need to write to the ULPI registers, I don't think we
> should try anything else except to use ULPI bus straight away. We'll

I'll agree with this.

> start by making use of ULPI bus possible by adding the quirk for BYT
> (attached), which to me is perfectly OK solution. I would appreciate
> if you gave it a review.

it's not perfectly ok for dwc3 to toggle PHY's GPIOs. Have the PHY
driver to that.

> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> index 8d95056..53902ea 100644
> --- a/drivers/usb/dwc3/dwc3-pci.c
> +++ b/drivers/usb/dwc3/dwc3-pci.c
> @@ -21,6 +21,7 @@
>  #include <linux/slab.h>
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
>  
>  #include "platform_data.h"
>  
> @@ -35,6 +36,24 @@
>  
>  static int dwc3_pci_quirks(struct pci_dev *pdev)
>  {
> +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> +	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> +		struct gpio_desc *gpio;
> +
> +		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> +		if (!IS_ERR(gpio)) {
> +			gpiod_direction_output(gpio, 0);
> +			gpiod_set_value_cansleep(gpio, 1);
> +			gpiod_put(gpio);
> +		}
> +		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> +		if (!IS_ERR(gpio)) {
> +			gpiod_direction_output(gpio, 0);
> +			gpiod_set_value_cansleep(gpio, 1);
> +			gpiod_put(gpio);
> +		}
> +	}

why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
very good.

-- 
balbi

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

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

[Felipe Balbi]

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

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

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

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

[David Cohen]

Hi Heikki, Felipe,

On Thu, Jan 29, 2015 at 10:20:23AM -0600, Felipe Balbi wrote:
> Hi
> 
> On Thu, Jan 29, 2015 at 04:14:12PM +0200, Heikki Krogerus wrote:
> > > > > > > Can you share how tusb1210 is connected on the platform you're using as
> > > > > > > test for this patch? I don't think this driver would work reliably with
> > > > > > > this device:
> > > > > > > http://liliputing.com/2014/11/trekstor-launches-first-android-tablet-based-intels-irda-reference-design.html
> > > > > > 
> > > > > > The only reason why that board doesn't work is because of very much
> > > > > > Baytrail-CR specific problems. These are are two issues, but the first
> > > > > 
> > > > > That's not BYT-CR specific problems. That's just dwc3 and tusb1210
> > > > > interacting as they're expecting to.
> > > > > 
> > > > > > one is critical for getting it working. Both will be handled, but
> > > > > > separately from this set:
> > > > > > 
> > > > > > 1) The firmware leaves the PHY in reset, forcing us to enable it
> > > > > > somehow in OS before accessing ulpi. Unless we can get a firmware fix
> > > > > > for that (it's starting to look like it's not going to happen; please
> > > > > > correct me if you know something else!), we need to add a quirk for
> > > > > > Baytrails (attached), which is probable still OK. But IMO this really
> > > > > > should be fixed in the firmware.
> > > > > 
> > > > > It seems you're expecting the PHY to be fully operational in order to
> > > > > probe it. That's wrong assumption. BYT-CR's BIOS is doing nothing wrong
> > > > > by leaving PHY on reset state.
> > > > 
> > > > But it is. If we want to use ULPI as a bus like we do, then the PHY
> > > > will be no different then devices attached to many other buses. We
> > > > have made firmware fixes like that before. All the devices need to be
> > > > in a state, operational enough after bootup, so we can probe them in
> > > > OS without the need for hacks where they are separately enabled before
> > > > it's possible.
> > > 
> > > That makes no sense. Not only dwc3 and phy could live as modules (which
> > > means they may probe far away from device's boot time) but we have
> > > examples of buses not behaving like you said. E.g. I2C needs master to
> > > be probed to have bus working and no BIOS needs to make I2C controller
> > > functional in order to probe its controller's driver.
> > 
> > You can't really compare a bus like i2c, which can't enumerate devices
> > natively, to ULPI which can.
> 
> why not ? The BIOS might not need to use the PHY (or USB) at all, it can
> very well decide to never turn it on, right ?
> 
> > > > > The real problem is what I stated above.
> > > > > With your current logic, you'll get stuck with checking/egg problem: you
> > > > > need phy probed to probe dwc3, but need dwc3 probed to power on phy.
> > > > > You need a logic to break this circular dependency.
> > > > 
> > > > The moment we register the ulpi interface with the ulpi bus in
> > > > dwc3_probe(), we know dwc3 has it's PHY interface in operational mode
> > > > and register access to ULPI PHY is possible. And that is all dwc3
> > > > needs to/can do.
> > > > 
> > > > I don't think you are seeing the whole "ulpi bus" in these patches,
> > > > but in any case; Like I said, this problem is purely BYT-CR specific,
> > > > which IMO really should be fixed in the firmware.
> > > 
> > > The proposed design has a flaw that breaks products on market simply
> > > because they don't have BIOS (unnecessarily) powering on phy. You're
> > > labeling that as BYT-CR specific issue because BYT-CR needs to be PM
> > > efficient and then it won't power on hw components in moment they don't
> > > need to. FW developers won't like this suggestion and I'd have to agree
> > > with them.
> > 
> > What exactly are we breaking here? The USB on BYT-CR does not work yet
> > with the mainline kernel, or does it? To enable it, I already
> > suggested the BYT quirk (attached again).

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

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

> 
> one comment below on this.
> 
> > I don't think the other boards we have which use TUSB1210, like the
> > BYT-I ones and I think some Merrifield based boards, expect any less
> > from PM efficiency then BYT-CR, but we don't need to do any tricks
> > with them in order to use ULPI bus. That is what I mean when I say
> > this is BYT-CR specific problem.
> 
> perhaps because firmware on those other boards are powering up the PHY ?

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

> 
> > I don't agree with PM arguments if it means that we should be ready to
> > accept loosing possibility for a generic solution in OS with a single
> > device like our PHY. I seriously doubt it would prevent the products
> > using these boards of achieving their PM requirements. But this
> > conversation is outside our topic.
> 
> we're not loosing anything. We're just considering what's the best way
> to tackle that ulpi_read() inside probe(). TUSB1210 driver _has_ to cope
> with situations where reset_gpio/cs_gpio are in unexpected state. Saying
> we will just "fix the firmware", as if that was a simple feat, is
> counter-productive.

Agree.

> 
> > > > > > 2) Since the gpio resources are given to the controller device in ACPI
> > > > > > tables and there isn't separate device object for the PHY at all, we
> > > > > > need to deliver the gpios somehow separately to the phy driver. There
> > > > > > is a thread where we are talking about how to do that:
> > > > > > https://lkml.org/lkml/2014/12/18/82
> > > > > 
> > > > > How about just leave the logic the way it is:
> > > > > dwc3-pci.c registers platform_device with gpio as resource if the GPIOs
> > > > > are provided to dwc3. If not, then dwc3-pci.c will expect phy to have
> > > > > its own ACPI id.
> > > > 
> > > > I think you are now talking about the platform devices for the legacy
> > > > USB PHY framework created in dwc3-pci.c, which btw. were removed.
> > > > Please note that we are not using platform bus with the ULPI devices,
> > > > and those devices are created by the bus driver and not dwc3.
> > > 
> > > Yes, that the one. Current products' BIOS on market didn't know about new
> > > ULPI bus. They rely on platform devices created by pci probe. Your ULPI
> > > bus proposal is way better to handle that problem and got my support
> > > since they beginning you showed that to me, but it does not justify
> > > breaking current working devices. Removing the platform device
> > > registration for phy with firmwares that rely on that was a mistake and
> > > any ACPI work related to fix that is unnecessary. These legacy ACPI
> > > tables gave the phy-related GPIOs to dwc3. Just mark is as legacy
> > > situation and let the legacy hw's happy. No vendor will change their
> > > BIOS after market due to non-buggy situation.
> > 
> > Well, I'm really not expecting any BIOS updates any more. I assumed
> > that was clear. Why else would I have started the whole planning of
> > the GPIO forwarding. But if it wasn't, then sorry. Now you know.
> > 
> > BYT-CR's USB is not supported in mainline yet unless I'm completely
> > mistaken, but we have the plan for it. Instead of trying to take any
> > shortcuts, let's follow that plan.

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

> > 
> > Because of the need to write to the ULPI registers, I don't think we
> > should try anything else except to use ULPI bus straight away. We'll
> 
> I'll agree with this.

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

> 
> > start by making use of ULPI bus possible by adding the quirk for BYT
> > (attached), which to me is perfectly OK solution. I would appreciate
> > if you gave it a review.
> 
> it's not perfectly ok for dwc3 to toggle PHY's GPIOs. Have the PHY
> driver to that.

Agree with that.

> 
> > diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> > index 8d95056..53902ea 100644
> > --- a/drivers/usb/dwc3/dwc3-pci.c
> > +++ b/drivers/usb/dwc3/dwc3-pci.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/pci.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/gpio/consumer.h>
> >  
> >  #include "platform_data.h"
> >  
> > @@ -35,6 +36,24 @@
> >  
> >  static int dwc3_pci_quirks(struct pci_dev *pdev)
> >  {
> > +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > +	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > +		struct gpio_desc *gpio;
> > +
> > +		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> > +		if (!IS_ERR(gpio)) {
> > +			gpiod_direction_output(gpio, 0);
> > +			gpiod_set_value_cansleep(gpio, 1);
> > +			gpiod_put(gpio);
> > +		}
> > +		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> > +		if (!IS_ERR(gpio)) {
> > +			gpiod_direction_output(gpio, 0);
> > +			gpiod_set_value_cansleep(gpio, 1);
> > +			gpiod_put(gpio);
> > +		}
> > +	}
> 
> why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
> very good.

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

Br, David

> 
> -- 
> balbi

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

[David Cohen]

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

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

[David Cohen]

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

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

[David Cohen]

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

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

[Heikki Krogerus]

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 <linux/slab.h>
> >  #include <linux/pci.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/gpio/consumer.h>
> >  
> >  #include "platform_data.h"
> >  
> > @@ -35,6 +36,24 @@
> >  
> >  static int dwc3_pci_quirks(struct pci_dev *pdev)
> >  {
> > +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > +	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > +		struct gpio_desc *gpio;
> > +
> > +		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> > +		if (!IS_ERR(gpio)) {
> > +			gpiod_direction_output(gpio, 0);
> > +			gpiod_set_value_cansleep(gpio, 1);
> > +			gpiod_put(gpio);
> > +		}
> > +		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> > +		if (!IS_ERR(gpio)) {
> > +			gpiod_direction_output(gpio, 0);
> > +			gpiod_set_value_cansleep(gpio, 1);
> > +			gpiod_put(gpio);
> > +		}
> > +	}
> 
> why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
> very good.

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

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

[Heikki Krogerus]

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

Re: [PATCH 7/8] phy: helpers for USB ULPI PHY registering

[Heikki Krogerus]

On Thu, Jan 29, 2015 at 10:11:24AM -0600, Felipe Balbi wrote:
> On Thu, Jan 29, 2015 at 04:34:41PM +0200, Heikki Krogerus wrote:
> > On Wed, Jan 28, 2015 at 11:03:55PM -0600, Felipe Balbi wrote:
> > > On Fri, Jan 23, 2015 at 05:12:57PM +0200, Heikki Krogerus wrote:
> > > > ULPI PHYs need to be bound to their controllers with a
> > > > lookup. This adds helpers that the ULPI drivers can use to
> > > > do both, the registration of the PHY and the lookup, at the
> > > > same time.
> > > > 
> > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > > > ---
> > > >  drivers/phy/ulpi_phy.h | 31 +++++++++++++++++++++++++++++++
> > > >  1 file changed, 31 insertions(+)
> > > >  create mode 100644 drivers/phy/ulpi_phy.h
> > > > 
> > > > diff --git a/drivers/phy/ulpi_phy.h b/drivers/phy/ulpi_phy.h
> > > > new file mode 100644
> > > > index 0000000..ac49fb6
> > > > --- /dev/null
> > > > +++ b/drivers/phy/ulpi_phy.h
> > > > @@ -0,0 +1,31 @@
> > > > +#include <linux/phy/phy.h>
> > > > +
> > > > +/**
> > > > + * Helper that registers PHY for a ULPI device and adds a lookup for binding it
> > > > + * and it's controller, which is always the parent.
> > > > + */
> > > > +static inline struct phy
> > > > +*ulpi_phy_create(struct ulpi *ulpi, struct phy_ops *ops)
> > > > +{
> > > > +	struct phy *phy;
> > > > +	int ret;
> > > > +
> > > > +	phy = phy_create(&ulpi->dev, NULL, ops);
> > > > +	if (IS_ERR(phy))
> > > > +		return phy;
> > > > +
> > > > +	ret = phy_create_lookup(phy, "usb2-phy", dev_name(ulpi->dev.parent));
> > > 
> > > I wonder if this would break DT-based boot where PHY phandle is
> > > available. Kishon ?
> > 
> > If there are of_node for the PHY, we will not use this binding at all.
> 
> where is that taken care of ? ulpi_phy_create() is called
> unconditionally.

When you call phy_get().


-- 
heikki

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

[Heikki Krogerus]

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

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

[-- Attachment #2: 0001-usb-dwc3-pci-remove-use-of-platform_data.patch --]
[-- Type: text/plain, Size: 2858 bytes --]

>From 0afd47616c03d268d1c0e2ad5f4108f7f1a2401d Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
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 <heikki.krogerus@linux.intel.com>
---
 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 <linux/pci.h>
 #include <linux/platform_device.h>
 
-#include "platform_data.h"
-
 /* FIXME define these in <linux/pci_ids.h> */
 #define PCI_VENDOR_ID_SYNOPSYS		0x16c3
 #define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3	0xabcd
@@ -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;
-
+		struct platform_device *dwc3 = pci_get_drvdata(pdev);
+		struct dev_gen_prop *prop;
+
+		prop = devm_kcalloc(&pdev->dev, 14, sizeof(*prop), GFP_KERNEL);
+		if (!prop)
+			return -ENOMEM;
+
+		prop[1].num = devm_kzalloc(&pdev->dev, sizeof(u8 *),
+					   GFP_KERNEL);
+		if (!prop[1].num)
+			return -ENOMEM;
+
+		prop[0].name = "snps,has-lpm-erratum";
+		prop[1].name = "snps,lpm-nyet-threshold";
+		prop[1].nval = 1;
+		prop[1].num[0] = 0xf;
+		prop[2].name = "snps,u2exit_lfps_quirk";
+		prop[3].name = "snps,u2ss_inp3_quirk";
+		prop[4].name = "snps,req_p1p2p3_quirk";
+		prop[5].name = "snps,del_p1p2p3_quirk";
+		prop[6].name = "snps,del_phy_power_chg_quirk";
+		prop[7].name = "snps,lfps_filter_quirk";
+		prop[8].name = "snps,rx_detect_poll_quirk";
+		prop[9].name = "snps,tx_de_emphasis_quirk";
 		/*
 		 * FIXME these quirks should be removed when AMD NL
 		 * taps out
 		 */
-		pdata.disable_scramble_quirk = true;
-		pdata.dis_u3_susphy_quirk = true;
-		pdata.dis_u2_susphy_quirk = true;
+		prop[10].name = "snps,disable_scramble_quirk";
+		prop[11].name = "snps,dis_u3_susphy_quirk";
+		prop[12].name = "snps,dis_u2_susphy_quirk";
 
-		return platform_device_add_data(pci_get_drvdata(pdev), &pdata,
-						sizeof(pdata));
+		dwc3->dev.gen_prop = prop;
 	}
 
 	return 0;
-- 
2.1.4

Re: [PATCH 7/8] phy: helpers for USB ULPI PHY registering

[Felipe Balbi]

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

On Fri, Jan 30, 2015 at 12:33:01PM +0200, Heikki Krogerus wrote:
> On Thu, Jan 29, 2015 at 10:11:24AM -0600, Felipe Balbi wrote:
> > On Thu, Jan 29, 2015 at 04:34:41PM +0200, Heikki Krogerus wrote:
> > > On Wed, Jan 28, 2015 at 11:03:55PM -0600, Felipe Balbi wrote:
> > > > On Fri, Jan 23, 2015 at 05:12:57PM +0200, Heikki Krogerus wrote:
> > > > > ULPI PHYs need to be bound to their controllers with a
> > > > > lookup. This adds helpers that the ULPI drivers can use to
> > > > > do both, the registration of the PHY and the lookup, at the
> > > > > same time.
> > > > > 
> > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > > > > ---
> > > > >  drivers/phy/ulpi_phy.h | 31 +++++++++++++++++++++++++++++++
> > > > >  1 file changed, 31 insertions(+)
> > > > >  create mode 100644 drivers/phy/ulpi_phy.h
> > > > > 
> > > > > diff --git a/drivers/phy/ulpi_phy.h b/drivers/phy/ulpi_phy.h
> > > > > new file mode 100644
> > > > > index 0000000..ac49fb6
> > > > > --- /dev/null
> > > > > +++ b/drivers/phy/ulpi_phy.h
> > > > > @@ -0,0 +1,31 @@
> > > > > +#include <linux/phy/phy.h>
> > > > > +
> > > > > +/**
> > > > > + * Helper that registers PHY for a ULPI device and adds a lookup for binding it
> > > > > + * and it's controller, which is always the parent.
> > > > > + */
> > > > > +static inline struct phy
> > > > > +*ulpi_phy_create(struct ulpi *ulpi, struct phy_ops *ops)
> > > > > +{
> > > > > +	struct phy *phy;
> > > > > +	int ret;
> > > > > +
> > > > > +	phy = phy_create(&ulpi->dev, NULL, ops);
> > > > > +	if (IS_ERR(phy))
> > > > > +		return phy;
> > > > > +
> > > > > +	ret = phy_create_lookup(phy, "usb2-phy", dev_name(ulpi->dev.parent));
> > > > 
> > > > I wonder if this would break DT-based boot where PHY phandle is
> > > > available. Kishon ?
> > > 
> > > If there are of_node for the PHY, we will not use this binding at all.
> > 
> > where is that taken care of ? ulpi_phy_create() is called
> > unconditionally.
> 
> When you call phy_get().

tks

-- 
balbi

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

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

[David Cohen]

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 <heikki.krogerus@linux.intel.com>
> 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 <heikki.krogerus@linux.intel.com>
> ---
>  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 <linux/pci.h>
>  #include <linux/platform_device.h>
>  
> -#include "platform_data.h"
> -
>  /* FIXME define these in <linux/pci_ids.h> */
>  #define PCI_VENDOR_ID_SYNOPSYS		0x16c3
>  #define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3	0xabcd
> @@ -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;
> -
> +		struct platform_device *dwc3 = pci_get_drvdata(pdev);
> +		struct dev_gen_prop *prop;
> +
> +		prop = devm_kcalloc(&pdev->dev, 14, sizeof(*prop), GFP_KERNEL);
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		prop[1].num = devm_kzalloc(&pdev->dev, sizeof(u8 *),
> +					   GFP_KERNEL);
> +		if (!prop[1].num)
> +			return -ENOMEM;
> +
> +		prop[0].name = "snps,has-lpm-erratum";
> +		prop[1].name = "snps,lpm-nyet-threshold";
> +		prop[1].nval = 1;
> +		prop[1].num[0] = 0xf;
> +		prop[2].name = "snps,u2exit_lfps_quirk";
> +		prop[3].name = "snps,u2ss_inp3_quirk";
> +		prop[4].name = "snps,req_p1p2p3_quirk";
> +		prop[5].name = "snps,del_p1p2p3_quirk";
> +		prop[6].name = "snps,del_phy_power_chg_quirk";
> +		prop[7].name = "snps,lfps_filter_quirk";
> +		prop[8].name = "snps,rx_detect_poll_quirk";
> +		prop[9].name = "snps,tx_de_emphasis_quirk";
>  		/*
>  		 * FIXME these quirks should be removed when AMD NL
>  		 * taps out
>  		 */
> -		pdata.disable_scramble_quirk = true;
> -		pdata.dis_u3_susphy_quirk = true;
> -		pdata.dis_u2_susphy_quirk = true;
> +		prop[10].name = "snps,disable_scramble_quirk";
> +		prop[11].name = "snps,dis_u3_susphy_quirk";
> +		prop[12].name = "snps,dis_u2_susphy_quirk";
>  
> -		return platform_device_add_data(pci_get_drvdata(pdev), &pdata,
> -						sizeof(pdata));
> +		dwc3->dev.gen_prop = prop;
>  	}
>  
>  	return 0;
> -- 
> 2.1.4
> 

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

[Felipe Balbi]

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

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 <linux/slab.h>
> > >  #include <linux/pci.h>
> > >  #include <linux/platform_device.h>
> > > +#include <linux/gpio/consumer.h>
> > >  
> > >  #include "platform_data.h"
> > >  
> > > @@ -35,6 +36,24 @@
> > >  
> > >  static int dwc3_pci_quirks(struct pci_dev *pdev)
> > >  {
> > > +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > > +	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > > +		struct gpio_desc *gpio;
> > > +
> > > +		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> > > +		if (!IS_ERR(gpio)) {
> > > +			gpiod_direction_output(gpio, 0);
> > > +			gpiod_set_value_cansleep(gpio, 1);
> > > +			gpiod_put(gpio);
> > > +		}
> > > +		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> > > +		if (!IS_ERR(gpio)) {
> > > +			gpiod_direction_output(gpio, 0);
> > > +			gpiod_set_value_cansleep(gpio, 1);
> > > +			gpiod_put(gpio);
> > > +		}
> > > +	}
> > 
> > why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
> > very good.
> 
> ..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(reset);

	...

	return 0;
}

This will have no effect on devices where PHY is already turned on and
will cope with the device David mentioned. If, however, there's a way to
get that eye diagram optimization without needing a ulpi_read() that's
*even* better, otherwise, above should fine in all cases.

cheers

-- 
balbi

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

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

[David Cohen]

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 <linux/slab.h>
> > >  #include <linux/pci.h>
> > >  #include <linux/platform_device.h>
> > > +#include <linux/gpio/consumer.h>
> > >  
> > >  #include "platform_data.h"
> > >  
> > > @@ -35,6 +36,24 @@
> > >  
> > >  static int dwc3_pci_quirks(struct pci_dev *pdev)
> > >  {
> > > +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > > +	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > > +		struct gpio_desc *gpio;
> > > +
> > > +		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> > > +		if (!IS_ERR(gpio)) {
> > > +			gpiod_direction_output(gpio, 0);
> > > +			gpiod_set_value_cansleep(gpio, 1);
> > > +			gpiod_put(gpio);
> > > +		}
> > > +		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> > > +		if (!IS_ERR(gpio)) {
> > > +			gpiod_direction_output(gpio, 0);
> > > +			gpiod_set_value_cansleep(gpio, 1);
> > > +			gpiod_put(gpio);
> > > +		}
> > > +	}
> > 
> > why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
> > very good.
> 
> ..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?

Br, David

> 
> 
> Thanks,
> 
> -- 
> heikki

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

[David Cohen]

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 <linux/slab.h>
> > > >  #include <linux/pci.h>
> > > >  #include <linux/platform_device.h>
> > > > +#include <linux/gpio/consumer.h>
> > > >  
> > > >  #include "platform_data.h"
> > > >  
> > > > @@ -35,6 +36,24 @@
> > > >  
> > > >  static int dwc3_pci_quirks(struct pci_dev *pdev)
> > > >  {
> > > > +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > > > +	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > > > +		struct gpio_desc *gpio;
> > > > +
> > > > +		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> > > > +		if (!IS_ERR(gpio)) {
> > > > +			gpiod_direction_output(gpio, 0);
> > > > +			gpiod_set_value_cansleep(gpio, 1);
> > > > +			gpiod_put(gpio);
> > > > +		}
> > > > +		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> > > > +		if (!IS_ERR(gpio)) {
> > > > +			gpiod_direction_output(gpio, 0);
> > > > +			gpiod_set_value_cansleep(gpio, 1);
> > > > +			gpiod_put(gpio);
> > > > +		}
> > > > +	}
> > > 
> > > why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
> > > very good.
> > 
> > ..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(reset);
> 
> 	...
> 
> 	return 0;
> }
> 
> This will have no effect on devices where PHY is already turned on and
> will cope with the device David mentioned. If, however, there's a way to
> get that eye diagram optimization without needing a ulpi_read() that's
> *even* better, otherwise, above should fine in all cases.

A comment here. The ulpi_read() works for eye diagram only and only if
there is no reset on PHY between boot and probe. It is assuming the BIOS
set the correct value and then we just save it right away. As soon as we
reset the PHY this value is lost. So the GPIO toggling breaks the
current eye diagram request proposal. IMHO this should really come from
_DSD which will maintain the value regardless to what happens with phy.

Br, David

> 
> cheers
> 
> -- 
> balbi

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

[Felipe Balbi]

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

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 <linux/slab.h>
> > > > >  #include <linux/pci.h>
> > > > >  #include <linux/platform_device.h>
> > > > > +#include <linux/gpio/consumer.h>
> > > > >  
> > > > >  #include "platform_data.h"
> > > > >  
> > > > > @@ -35,6 +36,24 @@
> > > > >  
> > > > >  static int dwc3_pci_quirks(struct pci_dev *pdev)
> > > > >  {
> > > > > +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > > > > +	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > > > > +		struct gpio_desc *gpio;
> > > > > +
> > > > > +		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> > > > > +		if (!IS_ERR(gpio)) {
> > > > > +			gpiod_direction_output(gpio, 0);
> > > > > +			gpiod_set_value_cansleep(gpio, 1);
> > > > > +			gpiod_put(gpio);
> > > > > +		}
> > > > > +		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> > > > > +		if (!IS_ERR(gpio)) {
> > > > > +			gpiod_direction_output(gpio, 0);
> > > > > +			gpiod_set_value_cansleep(gpio, 1);
> > > > > +			gpiod_put(gpio);
> > > > > +		}
> > > > > +	}
> > > > 
> > > > why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
> > > > very good.
> > > 
> > > ..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(reset);
> > 
> > 	...
> > 
> > 	return 0;
> > }
> > 
> > This will have no effect on devices where PHY is already turned on and
> > will cope with the device David mentioned. If, however, there's a way to
> > get that eye diagram optimization without needing a ulpi_read() that's
> > *even* better, otherwise, above should fine in all cases.
> 
> A comment here. The ulpi_read() works for eye diagram only and only if
> there is no reset on PHY between boot and probe. It is assuming the BIOS
> set the correct value and then we just save it right away. As soon as we
> reset the PHY this value is lost. So the GPIO toggling breaks the
> current eye diagram request proposal. IMHO this should really come from
> _DSD which will maintain the value regardless to what happens with phy.

fair enough.

-- 
balbi

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

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

[Felipe Balbi]

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

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

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

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

[Heikki Krogerus]

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

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

[Heikki Krogerus]

> > > > 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 what you are saying here?

> > > > 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 <linux/slab.h>
> > > >  #include <linux/pci.h>
> > > >  #include <linux/platform_device.h>
> > > > +#include <linux/gpio/consumer.h>
> > > >  
> > > >  #include "platform_data.h"
> > > >  
> > > > @@ -35,6 +36,24 @@
> > > >  
> > > >  static int dwc3_pci_quirks(struct pci_dev *pdev)
> > > >  {
> > > > +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > > > +	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > > > +		struct gpio_desc *gpio;
> > > > +
> > > > +		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> > > > +		if (!IS_ERR(gpio)) {
> > > > +			gpiod_direction_output(gpio, 0);
> > > > +			gpiod_set_value_cansleep(gpio, 1);
> > > > +			gpiod_put(gpio);
> > > > +		}
> > > > +		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> > > > +		if (!IS_ERR(gpio)) {
> > > > +			gpiod_direction_output(gpio, 0);
> > > > +			gpiod_set_value_cansleep(gpio, 1);
> > > > +			gpiod_put(gpio);
> > > > +		}
> > > > +	}
> > > 
> > > why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
> > > very good.
> > 
> > ..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.


Thanks,

-- 
heikki

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

[Heikki Krogerus]

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

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

[David Cohen]

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

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

[David Cohen]

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

> 
> > > > > 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 <linux/slab.h>
> > > > >  #include <linux/pci.h>
> > > > >  #include <linux/platform_device.h>
> > > > > +#include <linux/gpio/consumer.h>
> > > > >  
> > > > >  #include "platform_data.h"
> > > > >  
> > > > > @@ -35,6 +36,24 @@
> > > > >  
> > > > >  static int dwc3_pci_quirks(struct pci_dev *pdev)
> > > > >  {
> > > > > +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > > > > +	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > > > > +		struct gpio_desc *gpio;
> > > > > +
> > > > > +		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> > > > > +		if (!IS_ERR(gpio)) {
> > > > > +			gpiod_direction_output(gpio, 0);
> > > > > +			gpiod_set_value_cansleep(gpio, 1);
> > > > > +			gpiod_put(gpio);
> > > > > +		}
> > > > > +		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> > > > > +		if (!IS_ERR(gpio)) {
> > > > > +			gpiod_direction_output(gpio, 0);
> > > > > +			gpiod_set_value_cansleep(gpio, 1);
> > > > > +			gpiod_put(gpio);
> > > > > +		}
> > > > > +	}
> > > > 
> > > > why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
> > > > very good.
> > > 
> > > ..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.

This would bring support to current BYT-CR products, but it's a lot ugly :/
But current DSDT is a lot ugly too.

Felipe, we do need ULPI bus working for BYT-CR due to 2 scenarios:
- we need eye optimization
- we need to pull down D+/- on phy after cable disconnection so charging
  detection module can detect CDP connection during next cable plug in.

I see no way to make current BYT-CR platforms work with Heikki's ULPI
bus proposal without crippling it if we don't toggle these GPIOs outside
ULPI's device probe.

>From my side, the quirk is acceptable as a trade off to not damage his
design due to legacy platforms.

Br, David

> 
> 
> Thanks,
> 
> -- 
> heikki

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

[David Cohen]

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

Heikki, do you have a proposal for that? IMHO that's the main missing
point if we forget about BYT-CR legacy.

Br, David

> 
> > 
> > > > > > 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 <linux/slab.h>
> > > > > >  #include <linux/pci.h>
> > > > > >  #include <linux/platform_device.h>
> > > > > > +#include <linux/gpio/consumer.h>
> > > > > >  
> > > > > >  #include "platform_data.h"
> > > > > >  
> > > > > > @@ -35,6 +36,24 @@
> > > > > >  
> > > > > >  static int dwc3_pci_quirks(struct pci_dev *pdev)
> > > > > >  {
> > > > > > +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > > > > > +	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > > > > > +		struct gpio_desc *gpio;
> > > > > > +
> > > > > > +		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> > > > > > +		if (!IS_ERR(gpio)) {
> > > > > > +			gpiod_direction_output(gpio, 0);
> > > > > > +			gpiod_set_value_cansleep(gpio, 1);
> > > > > > +			gpiod_put(gpio);
> > > > > > +		}
> > > > > > +		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> > > > > > +		if (!IS_ERR(gpio)) {
> > > > > > +			gpiod_direction_output(gpio, 0);
> > > > > > +			gpiod_set_value_cansleep(gpio, 1);
> > > > > > +			gpiod_put(gpio);
> > > > > > +		}
> > > > > > +	}
> > > > > 
> > > > > why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
> > > > > very good.
> > > > 
> > > > ..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.
> 
> This would bring support to current BYT-CR products, but it's a lot ugly :/
> But current DSDT is a lot ugly too.
> 
> Felipe, we do need ULPI bus working for BYT-CR due to 2 scenarios:
> - we need eye optimization
> - we need to pull down D+/- on phy after cable disconnection so charging
>   detection module can detect CDP connection during next cable plug in.
> 
> I see no way to make current BYT-CR platforms work with Heikki's ULPI
> bus proposal without crippling it if we don't toggle these GPIOs outside
> ULPI's device probe.
> 
> From my side, the quirk is acceptable as a trade off to not damage his
> design due to legacy platforms.
> 
> Br, David
> 
> > 
> > 
> > Thanks,
> > 
> > -- 
> > heikki
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@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

[Heikki Krogerus]

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

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[David Cohen]

Hi Heikki,

On Fri, Jan 23, 2015 at 05:12:56PM +0200, Heikki Krogerus wrote:
> Registers DWC3's ULPI interface with the ULPI bus when it's
> available.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/dwc3/Kconfig  |   7 +++
>  drivers/usb/dwc3/Makefile |   4 ++
>  drivers/usb/dwc3/core.c   |   6 +++
>  drivers/usb/dwc3/core.h   |  13 ++++++
>  drivers/usb/dwc3/ulpi.c   | 112 ++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 142 insertions(+)
>  create mode 100644 drivers/usb/dwc3/ulpi.c
> 
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index 58b5b2c..cda82ad 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -11,6 +11,13 @@ config USB_DWC3
>  
>  if USB_DWC3
>  
> +config USB_DWC3_ULPI
> +	bool "Register ULPI PHY Interface"
> +	depends on USB_ULPI_BUS=y
> +	help
> +	  Select this if you have ULPI type PHY attached to your DWC3
> +	  controller.
> +
>  choice
>  	bool "DWC3 Mode Selection"
>  	default USB_DWC3_DUAL_ROLE if (USB && USB_GADGET)
> diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> index bb34fbc..2fc44e0 100644
> --- a/drivers/usb/dwc3/Makefile
> +++ b/drivers/usb/dwc3/Makefile
> @@ -16,6 +16,10 @@ ifneq ($(filter y,$(CONFIG_USB_DWC3_GADGET) $(CONFIG_USB_DWC3_DUAL_ROLE)),)
>  	dwc3-y				+= gadget.o ep0.o
>  endif
>  
> +ifneq ($(CONFIG_USB_DWC3_ULPI),)
> +	dwc3-y				+= ulpi.o
> +endif
> +
>  ifneq ($(CONFIG_DEBUG_FS),)
>  	dwc3-y				+= debugfs.o
>  endif
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index a8c9062..66cbf38 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, dwc);
>  	dwc3_cache_hwparams(dwc);
>  
> +	ret = dwc3_ulpi_init(dwc);

If I understood correctly, this call will result in enumerating the phy
via ULPI bus, then registering the correct ULPI device.
Can you guarantee ULPI will be always accessible at this point if we
remove dwc3 module and load it again?

Br, David

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

[David Cohen]

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

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[Heikki Krogerus]

> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index a8c9062..66cbf38 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev)
> >  	platform_set_drvdata(pdev, dwc);
> >  	dwc3_cache_hwparams(dwc);
> >  
> > +	ret = dwc3_ulpi_init(dwc);
> 
> If I understood correctly, this call will result in enumerating the phy
> via ULPI bus, then registering the correct ULPI device.
> Can you guarantee ULPI will be always accessible at this point if we
> remove dwc3 module and load it again?

OK, got it. So yes, I can guarantee that ULPI will be acessible at
this point. If we are in a state where we could soft reset dwc3, we
know we can access ULPI. The fact that dwc3 itself expects to be able
to write to the ULPI registers at that point guarantees it for us.

So in practice when ever dwc3 is powered we will be able to access
ULPI for as long as the USB2 PHY interface is not suspended separately
with GUSB2PHYCFG SusPHY bit. And even then we would only need to
resume it with the same bit and ULPI is accessible again.


Cheers,

-- 
heikki

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[David Cohen]

On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote:
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index a8c9062..66cbf38 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev)
> > >  	platform_set_drvdata(pdev, dwc);
> > >  	dwc3_cache_hwparams(dwc);
> > >  
> > > +	ret = dwc3_ulpi_init(dwc);
> > 
> > If I understood correctly, this call will result in enumerating the phy
> > via ULPI bus, then registering the correct ULPI device.
> > Can you guarantee ULPI will be always accessible at this point if we
> > remove dwc3 module and load it again?
> 
> OK, got it. So yes, I can guarantee that ULPI will be acessible at
> this point. If we are in a state where we could soft reset dwc3, we
> know we can access ULPI. The fact that dwc3 itself expects to be able
> to write to the ULPI registers at that point guarantees it for us.

I just double checked DWC3 TRM.
You are correct, by the time we're executing dwc3_core_soft_reset() ULPI
bus is already accessible. But the TRM also specifies an ULPI phy would
be reset by DCTL's core soft reset, which is executed by dwc3_core_init()
before calling dwc3_core_soft_reset(). It does mention DWC3 writes data
to an ULPI phy register during reset, but it also mentions the clock
sync happens during that time.

That makes no even OK, but more correct IMO to power on phy before
core's soft reset (i.e. by ACPI methods). But it lets us in a grey area
whether ULPI is reliably accessible before core's soft reset.

I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after
core's soft reset we've got no more grey area.

Br, David

> 
> So in practice when ever dwc3 is powered we will be able to access
> ULPI for as long as the USB2 PHY interface is not suspended separately
> with GUSB2PHYCFG SusPHY bit. And even then we would only need to
> resume it with the same bit and ULPI is accessible again.
> 
> 
> Cheers,
> 
> -- 
> heikki

Re: [PATCH 1/8] usb: add bus type for USB ULPI

[Stephen Boyd]

On 01/23/15 07:12, Heikki Krogerus wrote:
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index e614ef6..753cb08 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1176,6 +1176,19 @@ static int do_rio_entry(const char *filename,
>  }
>  ADD_TO_DEVTABLE("rapidio", rio_device_id, do_rio_entry);
>  
> +/* Looks like: mei:S */

This comment doesn't look right.

> +static int do_ulpi_entry(const char *filename, void *symval,
> +			 char *alias)
> +{
> +	DEF_FIELD(symval, ulpi_device_id, vendor);
> +	DEF_FIELD(symval, ulpi_device_id, product);
> +
> +	sprintf(alias, "ulpi:v%04xp%04x", vendor, product);
> +
> +	return 1;
> +}
> +ADD_TO_DEVTABLE("ulpi", ulpi_device_id, do_ulpi_entry);
> +

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[David Cohen]

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 <heikki.krogerus@linux.intel.com>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> ---

[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;
> +}
> +

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[David Cohen]

On Thu, Feb 12, 2015 at 05:41:30PM -0800, David Cohen wrote:
> On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote:
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index a8c9062..66cbf38 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev)
> > > >  	platform_set_drvdata(pdev, dwc);
> > > >  	dwc3_cache_hwparams(dwc);
> > > >  
> > > > +	ret = dwc3_ulpi_init(dwc);
> > > 
> > > If I understood correctly, this call will result in enumerating the phy
> > > via ULPI bus, then registering the correct ULPI device.
> > > Can you guarantee ULPI will be always accessible at this point if we
> > > remove dwc3 module and load it again?
> > 
> > OK, got it. So yes, I can guarantee that ULPI will be acessible at
> > this point. If we are in a state where we could soft reset dwc3, we
> > know we can access ULPI. The fact that dwc3 itself expects to be able
> > to write to the ULPI registers at that point guarantees it for us.
> 
> I just double checked DWC3 TRM.
> You are correct, by the time we're executing dwc3_core_soft_reset() ULPI
> bus is already accessible. But the TRM also specifies an ULPI phy would
> be reset by DCTL's core soft reset, which is executed by dwc3_core_init()
> before calling dwc3_core_soft_reset(). It does mention DWC3 writes data
> to an ULPI phy register during reset, but it also mentions the clock
> sync happens during that time.
> 
> That makes no even OK, but more correct IMO to power on phy before

Sorry for the typo. I meant:
That makes not only OK, but more correct...

> core's soft reset (i.e. by ACPI methods). But it lets us in a grey area
> whether ULPI is reliably accessible before core's soft reset.
> 
> I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after
> core's soft reset we've got no more grey area.
> 
> Br, David
> 
> > 
> > So in practice when ever dwc3 is powered we will be able to access
> > ULPI for as long as the USB2 PHY interface is not suspended separately
> > with GUSB2PHYCFG SusPHY bit. And even then we would only need to
> > resume it with the same bit and ULPI is accessible again.
> > 
> > 
> > Cheers,
> > 
> > -- 
> > heikki

Re: [PATCH 1/8] usb: add bus type for USB ULPI

[Heikki Krogerus]

On Thu, Feb 12, 2015 at 05:44:20PM -0800, Stephen Boyd wrote:
> On 01/23/15 07:12, Heikki Krogerus wrote:
> > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> > index e614ef6..753cb08 100644
> > --- a/scripts/mod/file2alias.c
> > +++ b/scripts/mod/file2alias.c
> > @@ -1176,6 +1176,19 @@ static int do_rio_entry(const char *filename,
> >  }
> >  ADD_TO_DEVTABLE("rapidio", rio_device_id, do_rio_entry);
> >  
> > +/* Looks like: mei:S */
> 
> This comment doesn't look right.

Oops. Thanks for catching that one.

> > +static int do_ulpi_entry(const char *filename, void *symval,
> > +			 char *alias)
> > +{
> > +	DEF_FIELD(symval, ulpi_device_id, vendor);
> > +	DEF_FIELD(symval, ulpi_device_id, product);
> > +
> > +	sprintf(alias, "ulpi:v%04xp%04x", vendor, product);
> > +
> > +	return 1;
> > +}
> > +ADD_TO_DEVTABLE("ulpi", ulpi_device_id, do_ulpi_entry);


-- 
heikki

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

[Heikki Krogerus]

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 <heikki.krogerus@linux.intel.com>
> > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > ---
> 
> [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

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[Heikki Krogerus]

On Thu, Feb 12, 2015 at 05:41:30PM -0800, David Cohen wrote:
> On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote:
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index a8c9062..66cbf38 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev)
> > > >  	platform_set_drvdata(pdev, dwc);
> > > >  	dwc3_cache_hwparams(dwc);
> > > >  
> > > > +	ret = dwc3_ulpi_init(dwc);
> > > 
> > > If I understood correctly, this call will result in enumerating the phy
> > > via ULPI bus, then registering the correct ULPI device.
> > > Can you guarantee ULPI will be always accessible at this point if we
> > > remove dwc3 module and load it again?
> > 
> > OK, got it. So yes, I can guarantee that ULPI will be acessible at
> > this point. If we are in a state where we could soft reset dwc3, we
> > know we can access ULPI. The fact that dwc3 itself expects to be able
> > to write to the ULPI registers at that point guarantees it for us.
> 
> I just double checked DWC3 TRM.
> You are correct, by the time we're executing dwc3_core_soft_reset() ULPI
> bus is already accessible. But the TRM also specifies an ULPI phy would
> be reset by DCTL's core soft reset, which is executed by dwc3_core_init()
> before calling dwc3_core_soft_reset(). It does mention DWC3 writes data
> to an ULPI phy register during reset, but it also mentions the clock
> sync happens during that time.
> 
> That makes no even OK, but more correct IMO to power on phy before
> core's soft reset (i.e. by ACPI methods). But it lets us in a grey area
> whether ULPI is reliably accessible before core's soft reset.
> 
> I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after
> core's soft reset we've got no more grey area.

Well, we have already requested the phys in dwc3_probe before that so
some refactoring will be needed. It's probable no a problem.

Btw, I'm sorry about telling this so late, but I'm going to be on
vacation for the next two weeks.


Cheers,

-- 
heikki

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

[Felipe Balbi]

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

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 <heikki.krogerus@linux.intel.com>
> > > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > > ---
> > 
> > [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

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

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

[David Cohen]

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 <linux/slab.h>
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
>  
>  #include "platform_data.h"
>  
> @@ -35,6 +36,24 @@
>  
>  static int dwc3_pci_quirks(struct pci_dev *pdev)
>  {
> +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> +	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> +		struct gpio_desc *gpio;
> +
> +		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> +		if (!IS_ERR(gpio)) {
> +			gpiod_direction_output(gpio, 0);
> +			gpiod_set_value_cansleep(gpio, 1);
> +			gpiod_put(gpio);
> +		}
> +		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> +		if (!IS_ERR(gpio)) {
> +			gpiod_direction_output(gpio, 0);
> +			gpiod_set_value_cansleep(gpio, 1);
> +			gpiod_put(gpio);
> +		}
> +	}
> +

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;

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[David Cohen]

Hi Heikki,

On Fri, Feb 13, 2015 at 03:16:40PM +0200, Heikki Krogerus wrote:
> On Thu, Feb 12, 2015 at 05:41:30PM -0800, David Cohen wrote:
> > On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote:
> > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > > index a8c9062..66cbf38 100644
> > > > > --- a/drivers/usb/dwc3/core.c
> > > > > +++ b/drivers/usb/dwc3/core.c
> > > > > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev)
> > > > >  	platform_set_drvdata(pdev, dwc);
> > > > >  	dwc3_cache_hwparams(dwc);
> > > > >  
> > > > > +	ret = dwc3_ulpi_init(dwc);
> > > > 
> > > > If I understood correctly, this call will result in enumerating the phy
> > > > via ULPI bus, then registering the correct ULPI device.
> > > > Can you guarantee ULPI will be always accessible at this point if we
> > > > remove dwc3 module and load it again?
> > > 
> > > OK, got it. So yes, I can guarantee that ULPI will be acessible at
> > > this point. If we are in a state where we could soft reset dwc3, we
> > > know we can access ULPI. The fact that dwc3 itself expects to be able
> > > to write to the ULPI registers at that point guarantees it for us.
> > 
> > I just double checked DWC3 TRM.
> > You are correct, by the time we're executing dwc3_core_soft_reset() ULPI
> > bus is already accessible. But the TRM also specifies an ULPI phy would
> > be reset by DCTL's core soft reset, which is executed by dwc3_core_init()
> > before calling dwc3_core_soft_reset(). It does mention DWC3 writes data
> > to an ULPI phy register during reset, but it also mentions the clock
> > sync happens during that time.
> > 
> > That makes no even OK, but more correct IMO to power on phy before
> > core's soft reset (i.e. by ACPI methods). But it lets us in a grey area
> > whether ULPI is reliably accessible before core's soft reset.
> > 
> > I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after
> > core's soft reset we've got no more grey area.
> 
> Well, we have already requested the phys in dwc3_probe before that so
> some refactoring will be needed. It's probable no a problem.

Sounds good :)

> 
> Btw, I'm sorry about telling this so late, but I'm going to be on
> vacation for the next two weeks.

Thanks for the notice.

Br, David

> 
> 
> Cheers,
> 
> -- 
> heikki

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

[Felipe Balbi]

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

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 <linux/slab.h>
> >  #include <linux/pci.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/gpio/consumer.h>
> >  
> >  #include "platform_data.h"
> >  
> > @@ -35,6 +36,24 @@
> >  
> >  static int dwc3_pci_quirks(struct pci_dev *pdev)
> >  {
> > +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > +	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > +		struct gpio_desc *gpio;
> > +
> > +		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> > +		if (!IS_ERR(gpio)) {
> > +			gpiod_direction_output(gpio, 0);
> > +			gpiod_set_value_cansleep(gpio, 1);
> > +			gpiod_put(gpio);
> > +		}
> > +		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> > +		if (!IS_ERR(gpio)) {
> > +			gpiod_direction_output(gpio, 0);
> > +			gpiod_set_value_cansleep(gpio, 1);
> > +			gpiod_put(gpio);
> > +		}
> > +	}
> > +
> 
> 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

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

Re: [PATCH 6/8] usb: dwc3: add ULPI interface support

[Felipe Balbi]

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

On Fri, Feb 13, 2015 at 02:03:45PM -0800, David Cohen wrote:
> Hi Heikki,
> 
> On Fri, Feb 13, 2015 at 03:16:40PM +0200, Heikki Krogerus wrote:
> > On Thu, Feb 12, 2015 at 05:41:30PM -0800, David Cohen wrote:
> > > On Thu, Feb 12, 2015 at 02:12:14PM +0200, Heikki Krogerus wrote:
> > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > > > index a8c9062..66cbf38 100644
> > > > > > --- a/drivers/usb/dwc3/core.c
> > > > > > +++ b/drivers/usb/dwc3/core.c
> > > > > > @@ -879,6 +879,10 @@ static int dwc3_probe(struct platform_device *pdev)
> > > > > >  	platform_set_drvdata(pdev, dwc);
> > > > > >  	dwc3_cache_hwparams(dwc);
> > > > > >  
> > > > > > +	ret = dwc3_ulpi_init(dwc);
> > > > > 
> > > > > If I understood correctly, this call will result in enumerating the phy
> > > > > via ULPI bus, then registering the correct ULPI device.
> > > > > Can you guarantee ULPI will be always accessible at this point if we
> > > > > remove dwc3 module and load it again?
> > > > 
> > > > OK, got it. So yes, I can guarantee that ULPI will be acessible at
> > > > this point. If we are in a state where we could soft reset dwc3, we
> > > > know we can access ULPI. The fact that dwc3 itself expects to be able
> > > > to write to the ULPI registers at that point guarantees it for us.
> > > 
> > > I just double checked DWC3 TRM.
> > > You are correct, by the time we're executing dwc3_core_soft_reset() ULPI
> > > bus is already accessible. But the TRM also specifies an ULPI phy would
> > > be reset by DCTL's core soft reset, which is executed by dwc3_core_init()
> > > before calling dwc3_core_soft_reset(). It does mention DWC3 writes data
> > > to an ULPI phy register during reset, but it also mentions the clock
> > > sync happens during that time.
> > > 
> > > That makes no even OK, but more correct IMO to power on phy before
> > > core's soft reset (i.e. by ACPI methods). But it lets us in a grey area
> > > whether ULPI is reliably accessible before core's soft reset.
> > > 
> > > I believe if you move the dwc3_ulpi_init() to dwc3_core_init(), after
> > > core's soft reset we've got no more grey area.
> > 
> > Well, we have already requested the phys in dwc3_probe before that so
> > some refactoring will be needed. It's probable no a problem.
> 
> Sounds good :)
> 
> > 
> > Btw, I'm sorry about telling this so late, but I'm going to be on
> > vacation for the next two weeks.
> 
> Thanks for the notice.

you'll be back right around merge window closing time :-) Not to worry,
we'll have time to get this into the next merge window.

-- 
balbi

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

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

[David Cohen]

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 <linux/slab.h>
> > >  #include <linux/pci.h>
> > >  #include <linux/platform_device.h>
> > > +#include <linux/gpio/consumer.h>
> > >  
> > >  #include "platform_data.h"
> > >  
> > > @@ -35,6 +36,24 @@
> > >  
> > >  static int dwc3_pci_quirks(struct pci_dev *pdev)
> > >  {
> > > +	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > > +	    pdev->device == PCI_DEVICE_ID_INTEL_BYT) {
> > > +		struct gpio_desc *gpio;
> > > +
> > > +		gpio = gpiod_get_index(&pdev->dev, "reset", 0);
> > > +		if (!IS_ERR(gpio)) {
> > > +			gpiod_direction_output(gpio, 0);
> > > +			gpiod_set_value_cansleep(gpio, 1);
> > > +			gpiod_put(gpio);
> > > +		}
> > > +		gpio = gpiod_get_index(&pdev->dev, "cs", 1);
> > > +		if (!IS_ERR(gpio)) {
> > > +			gpiod_direction_output(gpio, 0);
> > > +			gpiod_set_value_cansleep(gpio, 1);
> > > +			gpiod_put(gpio);
> > > +		}
> > > +	}
> > > +
> > 
> > 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