[PATCH 0/3] phy: ulpi bus

[PATCH 0/3] phy: ulpi bus

[Heikki Krogerus]

Heikki Krogerus (3):
  phy: add bus for USB ULPI PHYs
  usb: dwc3: add ULPI interface support
  phy: ulpi: add driver for TI TUSB1210

 MAINTAINERS                        |   7 ++
 drivers/phy/Kconfig                |   2 +
 drivers/phy/Makefile               |   1 +
 drivers/phy/ulpi/Kconfig           |  20 +++
 drivers/phy/ulpi/Makefile          |   4 +
 drivers/phy/ulpi/tusb1210.c        | 131 ++++++++++++++++++++
 drivers/phy/ulpi/ulpi.c            | 246 +++++++++++++++++++++++++++++++++++++
 drivers/phy/ulpi/ulpi_phy.h        |  31 +++++
 drivers/usb/dwc3/Kconfig           |   7 ++
 drivers/usb/dwc3/Makefile          |   4 +
 drivers/usb/dwc3/core.c            |   9 +-
 drivers/usb/dwc3/core.h            |  22 ++++
 drivers/usb/dwc3/ulpi.c            | 102 +++++++++++++++
 include/linux/phy/ulpi/driver.h    |  58 +++++++++
 include/linux/phy/ulpi/interface.h |  23 ++++
 include/linux/phy/ulpi/regs.h      | 130 ++++++++++++++++++++
 include/linux/usb/ulpi.h           | 135 +-------------------
 17 files changed, 799 insertions(+), 133 deletions(-)
 create mode 100644 drivers/phy/ulpi/Kconfig
 create mode 100644 drivers/phy/ulpi/Makefile
 create mode 100644 drivers/phy/ulpi/tusb1210.c
 create mode 100644 drivers/phy/ulpi/ulpi.c
 create mode 100644 drivers/phy/ulpi/ulpi_phy.h
 create mode 100644 drivers/usb/dwc3/ulpi.c
 create mode 100644 include/linux/phy/ulpi/driver.h
 create mode 100644 include/linux/phy/ulpi/interface.h
 create mode 100644 include/linux/phy/ulpi/regs.h

-- 
2.1.4

[PATCH 1/3] phy: add bus for USB ULPI PHYs

[Heikki Krogerus]

UTMI+ Low Pin Interface (ULPI) is a commonly used PHY
interface for USB 2.0. It describe a standard set of
registers which the vendors can extend for their specific
needs.

ULPI registers are accessed from the controller. The purpose
of the bus is to provide nice way for the controller drivers
to share that access with the actual PHY drivers.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 MAINTAINERS                        |   7 ++
 drivers/phy/Kconfig                |   2 +
 drivers/phy/Makefile               |   1 +
 drivers/phy/ulpi/Kconfig           |   9 ++
 drivers/phy/ulpi/Makefile          |   2 +
 drivers/phy/ulpi/ulpi.c            | 246 +++++++++++++++++++++++++++++++++++++
 drivers/phy/ulpi/ulpi_phy.h        |  31 +++++
 include/linux/phy/ulpi/driver.h    |  58 +++++++++
 include/linux/phy/ulpi/interface.h |  23 ++++
 include/linux/phy/ulpi/regs.h      | 130 ++++++++++++++++++++
 include/linux/usb/ulpi.h           | 135 +-------------------
 11 files changed, 512 insertions(+), 132 deletions(-)
 create mode 100644 drivers/phy/ulpi/Kconfig
 create mode 100644 drivers/phy/ulpi/Makefile
 create mode 100644 drivers/phy/ulpi/ulpi.c
 create mode 100644 drivers/phy/ulpi/ulpi_phy.h
 create mode 100644 include/linux/phy/ulpi/driver.h
 create mode 100644 include/linux/phy/ulpi/interface.h
 create mode 100644 include/linux/phy/ulpi/regs.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 4ef032a..fce81a4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4233,6 +4233,13 @@ S:	Supported
 F:	drivers/phy/
 F:	include/linux/phy/
 
+ULPI PHY BUS
+M:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+L:	linux-usb@vger.kernel.org
+S:	Supported
+F:	drivers/phy/ulpi/
+F:	include/linux/phy/ulpi/
+
 GENERIC UIO DRIVER FOR PCI DEVICES
 M:	"Michael S. Tsirkin" <mst@redhat.com>
 L:	kvm@vger.kernel.org
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index ccad880..9bfb556 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -277,4 +277,6 @@ config PHY_STIH41X_USB
 	  Enable this to support the USB transceiver that is part of
 	  STMicroelectronics STiH41x SoC series.
 
+source "drivers/phy/ulpi/Kconfig"
+
 endmenu
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index aa74f96..8914f3d 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -34,3 +34,4 @@ obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY)	+= phy-spear1340-miphy.o
 obj-$(CONFIG_PHY_XGENE)			+= phy-xgene.o
 obj-$(CONFIG_PHY_STIH407_USB)		+= phy-stih407-usb.o
 obj-$(CONFIG_PHY_STIH41X_USB)		+= phy-stih41x-usb.o
+obj-$(CONFIG_ULPI_PHY)			+= ulpi/
diff --git a/drivers/phy/ulpi/Kconfig b/drivers/phy/ulpi/Kconfig
new file mode 100644
index 0000000..8007df2
--- /dev/null
+++ b/drivers/phy/ulpi/Kconfig
@@ -0,0 +1,9 @@
+
+config ULPI_PHY
+	bool "USB ULPI PHY interface support"
+	depends on USB || USB_GADGET
+	select GENERIC_PHY
+	help
+	  Say yes if you have ULPI PHY attached to your USB controller.
+
+	  If unsure, say N.
diff --git a/drivers/phy/ulpi/Makefile b/drivers/phy/ulpi/Makefile
new file mode 100644
index 0000000..59e61cb
--- /dev/null
+++ b/drivers/phy/ulpi/Makefile
@@ -0,0 +1,2 @@
+ulpiphy-y			:= ulpi.o
+obj-$(CONFIG_ULPI_PHY)		+= ulpiphy.o
diff --git a/drivers/phy/ulpi/ulpi.c b/drivers/phy/ulpi/ulpi.c
new file mode 100644
index 0000000..4a05dd5
--- /dev/null
+++ b/drivers/phy/ulpi/ulpi.c
@@ -0,0 +1,246 @@
+/**
+ * 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/phy/ulpi/interface.h>
+#include <linux/phy/ulpi/driver.h>
+#include <linux/phy/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_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,
+	.probe = ulpi_probe,
+	.remove = ulpi_remove,
+};
+
+/**
+ * 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 void ulpi_dev_release(struct device *dev)
+{
+	kfree(to_ulpi_dev(dev));
+}
+
+static ssize_t
+vendor_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct ulpi *ulpi = to_ulpi_dev(dev);
+
+	return sprintf(buf, "%04x\n", ulpi->id.vendor);
+}
+static DEVICE_ATTR_RO(vendor);
+
+static ssize_t
+product_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct ulpi *ulpi = to_ulpi_dev(dev);
+
+	return sprintf(buf, "%04x\n", ulpi->id.product);
+}
+static DEVICE_ATTR_RO(product);
+
+static struct attribute *ulpi_dev_attrs[] = {
+	&dev_attr_vendor.attr,
+	&dev_attr_product.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
+};
+
+struct device_type ulpi_device_type = {
+	.name = "ulpi_device",
+	.groups = ulpi_dev_attr_groups,
+	.release = ulpi_dev_release,
+};
+
+/* -------------------------------------------------------------------------- */
+
+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));
+
+	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");
+MODULE_DESCRIPTION("USB ULPI PHY bus");
diff --git a/drivers/phy/ulpi/ulpi_phy.h b/drivers/phy/ulpi/ulpi_phy.h
new file mode 100644
index 0000000..ac49fb6
--- /dev/null
+++ b/drivers/phy/ulpi/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);
+}
diff --git a/include/linux/phy/ulpi/driver.h b/include/linux/phy/ulpi/driver.h
new file mode 100644
index 0000000..639a462
--- /dev/null
+++ b/include/linux/phy/ulpi/driver.h
@@ -0,0 +1,58 @@
+#ifndef __DRIVERS_PHY_ULPI_H
+#define __DRIVERS_PHY_ULPI_H
+
+#include <linux/device.h>
+#include <linux/types.h>
+
+struct ulpi_ops;
+
+struct ulpi_device_id {
+	__u16 vendor;
+	__u16 product;
+};
+
+/**
+ * struct ulpi - describes ULPI PHY device
+ * @id: vendor and product ids for ULPI device
+ * @ops: I/O access
+ * @dev: device interface
+ * @priv: for drivers private use
+ */
+struct ulpi {
+	struct ulpi_device_id id;
+	struct ulpi_ops *ops;
+	struct device dev;
+	void *priv;
+};
+
+#define to_ulpi_dev(d) container_of(d, struct 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 {
+	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 /* __DRIVERS_PHY_ULPI_H */
diff --git a/include/linux/phy/ulpi/interface.h b/include/linux/phy/ulpi/interface.h
new file mode 100644
index 0000000..51569fe
--- /dev/null
+++ b/include/linux/phy/ulpi/interface.h
@@ -0,0 +1,23 @@
+#ifndef __DRIVERS_PHY_ULPI_INTERFACE_H
+#define __DRIVERS_PHY_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 /* __DRIVERS_PHY_ULPI_INTERFACE_H */
diff --git a/include/linux/phy/ulpi/regs.h b/include/linux/phy/ulpi/regs.h
new file mode 100644
index 0000000..8407df7
--- /dev/null
+++ b/include/linux/phy/ulpi/regs.h
@@ -0,0 +1,130 @@
+#ifndef __DRIVERS_PHY_ULPI_REGS_H
+#define __DRIVERS_PHY_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 /* __DRIVERS_PHY_ULPI_REGS_H */
diff --git a/include/linux/usb/ulpi.h b/include/linux/usb/ulpi.h
index 5c295c2..f5518e4 100644
--- a/include/linux/usb/ulpi.h
+++ b/include/linux/usb/ulpi.h
@@ -12,6 +12,9 @@
 #define __LINUX_USB_ULPI_H
 
 #include <linux/usb/otg.h>
+
+#include "../phy/ulpi/regs.h"
+
 /*-------------------------------------------------------------------------*/
 
 /*
@@ -49,138 +52,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);
-- 
2.1.4

[PATCH 2/3] usb: dwc3: add ULPI interface support

[Heikki Krogerus]

Registers ULPI interface with the ULPI bus if HSPHY type is
ULPI.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Felipe Balbi <balbi@ti.com>
---
 drivers/usb/dwc3/Kconfig  |   7 ++++
 drivers/usb/dwc3/Makefile |   4 ++
 drivers/usb/dwc3/core.c   |   9 +++-
 drivers/usb/dwc3/core.h   |  22 ++++++++++
 drivers/usb/dwc3/ulpi.c   | 102 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 143 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/dwc3/ulpi.c

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 58b5b2c..6d0c5e6 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 "Provide ULPI PHY Interface"
+	depends on ULPI_PHY=y || ULPI_PHY=USB_DWC3
+	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 25ddc39..5219bc7 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -876,12 +876,17 @@ 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_ulpi_init(dwc);
+	if (ret)
+		return ret;
+
 	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;
@@ -965,6 +970,7 @@ err1:
 
 err0:
 	dwc3_free_event_buffers(dwc);
+	dwc3_ulpi_exit(dwc);
 
 	return ret;
 }
@@ -984,6 +990,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 0842aa8..f6881a6 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -32,6 +32,7 @@
 #include <linux/usb/otg.h>
 
 #include <linux/phy/phy.h>
+#include <linux/phy/ulpi/interface.h>
 
 #define DWC3_MSG_MAX	500
 
@@ -174,6 +175,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)
@@ -590,6 +599,7 @@ struct dwc3_hwparams {
 #define DWC3_NUM_INT(n)		(((n) & (0x3f << 15)) >> 15)
 
 /* HWPARAMS3 */
+#define DWC3_ULPI_HSPHY		(1 << 3)
 #define DWC3_NUM_IN_EPS_MASK	(0x1f << 18)
 #define DWC3_NUM_EPS_MASK	(0x3f << 12)
 #define DWC3_NUM_EPS(p)		(((p)->hwparams3 &		\
@@ -739,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;
 
@@ -1035,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..ee3ebbe
--- /dev/null
+++ b/drivers/usb/dwc3/ulpi.c
@@ -0,0 +1,102 @@
+/**
+ * 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/phy/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;
+	}
+
+	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)
+{
+	int ret;
+
+	/* First check if there is ULPI PHY */
+	ret = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
+	if (!(ret & DWC3_ULPI_HSPHY))
+		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

Re: [PATCH 2/3] usb: dwc3: add ULPI interface support

[Felipe Balbi]

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

Hi,

On Tue, Jan 20, 2015 at 11:18:21AM +0200, Heikki Krogerus wrote:
> Registers ULPI interface with the ULPI bus if HSPHY type is
> ULPI.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Felipe Balbi <balbi@ti.com>

you're doing quite a bit in a single patch...

> ---
>  drivers/usb/dwc3/Kconfig  |   7 ++++
>  drivers/usb/dwc3/Makefile |   4 ++
>  drivers/usb/dwc3/core.c   |   9 +++-
>  drivers/usb/dwc3/core.h   |  22 ++++++++++
>  drivers/usb/dwc3/ulpi.c   | 102 ++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 143 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/usb/dwc3/ulpi.c
> 
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index 58b5b2c..6d0c5e6 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 "Provide ULPI PHY Interface"
> +	depends on ULPI_PHY=y || ULPI_PHY=USB_DWC3
> +	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 25ddc39..5219bc7 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -876,12 +876,17 @@ 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_ulpi_init(dwc);
> +	if (ret)
> +		return ret;
> +
>  	ret = dwc3_core_get_phy(dwc);
>  	if (ret)
>  		return ret;
>  
>  	spin_lock_init(&dwc->lock);
> -	platform_set_drvdata(pdev, dwc);

why do you need to move this ? Looks like this should be a cleanup and
split into a single patch.

it also appears that you need another patch moving dwc3_cache_hwparams()
before all of these other calls, so you can use it from
dwc3_ulpi_init().

> @@ -965,6 +970,7 @@ err1:
>  
>  err0:
>  	dwc3_free_event_buffers(dwc);
> +	dwc3_ulpi_exit(dwc);
>  
>  	return ret;
>  }
> @@ -984,6 +990,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 0842aa8..f6881a6 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -32,6 +32,7 @@
>  #include <linux/usb/otg.h>
>  
>  #include <linux/phy/phy.h>
> +#include <linux/phy/ulpi/interface.h>
>  
>  #define DWC3_MSG_MAX	500
>  
> @@ -174,6 +175,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)

separate patch

> @@ -590,6 +599,7 @@ struct dwc3_hwparams {
>  #define DWC3_NUM_INT(n)		(((n) & (0x3f << 15)) >> 15)
>  
>  /* HWPARAMS3 */
> +#define DWC3_ULPI_HSPHY		(1 << 3)

you also need a patch which defines this bit of HWPARAMS3. This is also
the wrong definition. Which core revision do you have ? I can see that
bit 3 is part of a 2 bit field called:

DWC_USB3_HSPHY_INTERFACE

moreover, there are systems which have both ULPI and UTMI enabled and
you can't really know which one the PHY is using.

This needs a bit more thought.

>  #define DWC3_NUM_IN_EPS_MASK	(0x1f << 18)
>  #define DWC3_NUM_EPS_MASK	(0x3f << 12)
>  #define DWC3_NUM_EPS(p)		(((p)->hwparams3 &		\
> @@ -739,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;
>  
> @@ -1035,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..ee3ebbe
> --- /dev/null
> +++ b/drivers/usb/dwc3/ulpi.c
> @@ -0,0 +1,102 @@
> +/**
> + * 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/phy/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;

this could use a 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)
> +{
> +	int ret;
> +
> +	/* First check if there is ULPI PHY */
> +	ret = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
> +	if (!(ret & DWC3_ULPI_HSPHY))
> +		return 0;

should use the cached structure.

> +	/* 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
> 

-- 
balbi

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

Re: [PATCH 2/3] usb: dwc3: add ULPI interface support

[Heikki Krogerus]

On Tue, Jan 20, 2015 at 09:23:37AM -0600, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Jan 20, 2015 at 11:18:21AM +0200, Heikki Krogerus wrote:
> > Registers ULPI interface with the ULPI bus if HSPHY type is
> > ULPI.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Felipe Balbi <balbi@ti.com>
> 
> you're doing quite a bit in a single patch...
> 
> > ---
> >  drivers/usb/dwc3/Kconfig  |   7 ++++
> >  drivers/usb/dwc3/Makefile |   4 ++
> >  drivers/usb/dwc3/core.c   |   9 +++-
> >  drivers/usb/dwc3/core.h   |  22 ++++++++++
> >  drivers/usb/dwc3/ulpi.c   | 102 ++++++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 143 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/usb/dwc3/ulpi.c
> > 
> > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> > index 58b5b2c..6d0c5e6 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 "Provide ULPI PHY Interface"
> > +	depends on ULPI_PHY=y || ULPI_PHY=USB_DWC3
> > +	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 25ddc39..5219bc7 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -876,12 +876,17 @@ 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_ulpi_init(dwc);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = dwc3_core_get_phy(dwc);
> >  	if (ret)
> >  		return ret;
> >  
> >  	spin_lock_init(&dwc->lock);
> > -	platform_set_drvdata(pdev, dwc);
> 
> why do you need to move this ? Looks like this should be a cleanup and
> split into a single patch.

OK.

> it also appears that you need another patch moving dwc3_cache_hwparams()
> before all of these other calls, so you can use it from
> dwc3_ulpi_init().

OK.

> > @@ -965,6 +970,7 @@ err1:
> >  
> >  err0:
> >  	dwc3_free_event_buffers(dwc);
> > +	dwc3_ulpi_exit(dwc);
> >  
> >  	return ret;
> >  }
> > @@ -984,6 +990,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 0842aa8..f6881a6 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -32,6 +32,7 @@
> >  #include <linux/usb/otg.h>
> >  
> >  #include <linux/phy/phy.h>
> > +#include <linux/phy/ulpi/interface.h>
> >  
> >  #define DWC3_MSG_MAX	500
> >  
> > @@ -174,6 +175,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)
> 
> separate patch

OK.

> > @@ -590,6 +599,7 @@ struct dwc3_hwparams {
> >  #define DWC3_NUM_INT(n)		(((n) & (0x3f << 15)) >> 15)
> >  
> >  /* HWPARAMS3 */
> > +#define DWC3_ULPI_HSPHY		(1 << 3)
> 
> you also need a patch which defines this bit of HWPARAMS3. This is also
> the wrong definition. Which core revision do you have ? I can see that
> bit 3 is part of a 2 bit field called:
> 
> DWC_USB3_HSPHY_INTERFACE

I have the same in my databook. I agree, it's not good like that.

> moreover, there are systems which have both ULPI and UTMI enabled and
> you can't really know which one the PHY is using.
> 
> This needs a bit more thought.

Sure, I'll think of something better for this.

> >  #define DWC3_NUM_IN_EPS_MASK	(0x1f << 18)
> >  #define DWC3_NUM_EPS_MASK	(0x3f << 12)
> >  #define DWC3_NUM_EPS(p)		(((p)->hwparams3 &		\
> > @@ -739,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;
> >  
> > @@ -1035,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..ee3ebbe
> > --- /dev/null
> > +++ b/drivers/usb/dwc3/ulpi.c
> > @@ -0,0 +1,102 @@
> > +/**
> > + * 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/phy/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;
> 
> this could use a cpu_relax();

Agreed.

> > +	}
> > +
> > +	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)
> > +{
> > +	int ret;
> > +
> > +	/* First check if there is ULPI PHY */
> > +	ret = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
> > +	if (!(ret & DWC3_ULPI_HSPHY))
> > +		return 0;
> 
> should use the cached structure.

Sure.


Thanks,

-- 
heikki

[PATCH 3/3] phy: ulpi: add driver for TI TUSB1210

[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>
---
 drivers/phy/ulpi/Kconfig    |  11 ++++
 drivers/phy/ulpi/Makefile   |   2 +
 drivers/phy/ulpi/tusb1210.c | 131 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 144 insertions(+)
 create mode 100644 drivers/phy/ulpi/tusb1210.c

diff --git a/drivers/phy/ulpi/Kconfig b/drivers/phy/ulpi/Kconfig
index 8007df2..7cd6f82 100644
--- a/drivers/phy/ulpi/Kconfig
+++ b/drivers/phy/ulpi/Kconfig
@@ -7,3 +7,14 @@ config ULPI_PHY
 	  Say yes if you have ULPI PHY attached to your USB controller.
 
 	  If unsure, say N.
+
+if ULPI_PHY
+
+config ULPI_TUSB1210
+	tristate "TI TUSB1210 USB PHY module"
+	depends on POWER_SUPPLY
+	select USB_PHY
+	help
+	  Support for TI TUSB1210 USB ULPI PHY.
+
+endif
diff --git a/drivers/phy/ulpi/Makefile b/drivers/phy/ulpi/Makefile
index 59e61cb..7ee6679 100644
--- a/drivers/phy/ulpi/Makefile
+++ b/drivers/phy/ulpi/Makefile
@@ -1,2 +1,4 @@
 ulpiphy-y			:= ulpi.o
 obj-$(CONFIG_ULPI_PHY)		+= ulpiphy.o
+
+obj-$(CONFIG_ULPI_TUSB1210)	+= tusb1210.o
diff --git a/drivers/phy/ulpi/tusb1210.c b/drivers/phy/ulpi/tusb1210.c
new file mode 100644
index 0000000..ac77f98
--- /dev/null
+++ b/drivers/phy/ulpi/tusb1210.c
@@ -0,0 +1,131 @@
+/**
+ * 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/phy/ulpi/driver.h>
+#include <linux/phy/ulpi/regs.h>
+#include <linux/gpio/consumer.h>
+
+#include "ulpi_phy.h"
+
+struct tusb1210 {
+	struct ulpi *ulpi;
+	struct phy *phy;
+	struct gpio_desc *gpio_reset;
+	struct gpio_desc *gpio_cs;
+	u8 ctx[1];
+};
+
+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 optimisation value */
+	ulpi_write(tusb->ulpi, ULPI_EXT_VENDOR_SPECIFIC, tusb->ctx[0]);
+
+	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, ULPI_EXT_VENDOR_SPECIFIC);
+	if (ret < 0)
+		return ret;
+
+	tusb->ctx[0] = 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);
+	dev_set_drvdata(&ulpi->dev, tusb);
+	return 0;
+}
+
+static void tusb1210_remove(struct ulpi *ulpi)
+{
+	struct tusb1210 *tusb = dev_get_drvdata(&ulpi->dev);
+
+	ulpi_phy_destroy(ulpi, tusb->phy);
+}
+
+#define TI_VENDOR_ID 0x0451
+
+static 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");
+MODULE_DESCRIPTION("TUSB1210 ULPI PHY driver");
-- 
2.1.4

Re: [PATCH 3/3] phy: ulpi: add driver for TI TUSB1210

[Felipe Balbi]

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

Hi,

On Tue, Jan 20, 2015 at 11:18:22AM +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>
> ---
>  drivers/phy/ulpi/Kconfig    |  11 ++++
>  drivers/phy/ulpi/Makefile   |   2 +
>  drivers/phy/ulpi/tusb1210.c | 131 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 144 insertions(+)
>  create mode 100644 drivers/phy/ulpi/tusb1210.c
> 
> diff --git a/drivers/phy/ulpi/Kconfig b/drivers/phy/ulpi/Kconfig
> index 8007df2..7cd6f82 100644
> --- a/drivers/phy/ulpi/Kconfig
> +++ b/drivers/phy/ulpi/Kconfig
> @@ -7,3 +7,14 @@ config ULPI_PHY
>  	  Say yes if you have ULPI PHY attached to your USB controller.
>  
>  	  If unsure, say N.
> +
> +if ULPI_PHY
> +
> +config ULPI_TUSB1210
> +	tristate "TI TUSB1210 USB PHY module"
> +	depends on POWER_SUPPLY
> +	select USB_PHY
> +	help
> +	  Support for TI TUSB1210 USB ULPI PHY.
> +
> +endif
> diff --git a/drivers/phy/ulpi/Makefile b/drivers/phy/ulpi/Makefile
> index 59e61cb..7ee6679 100644
> --- a/drivers/phy/ulpi/Makefile
> +++ b/drivers/phy/ulpi/Makefile
> @@ -1,2 +1,4 @@
>  ulpiphy-y			:= ulpi.o
>  obj-$(CONFIG_ULPI_PHY)		+= ulpiphy.o
> +
> +obj-$(CONFIG_ULPI_TUSB1210)	+= tusb1210.o
> diff --git a/drivers/phy/ulpi/tusb1210.c b/drivers/phy/ulpi/tusb1210.c
> new file mode 100644
> index 0000000..ac77f98
> --- /dev/null
> +++ b/drivers/phy/ulpi/tusb1210.c

do you really need this extra ulpi directory ?

I wonder if phy-tusb1210.c as a name would be enough.

> @@ -0,0 +1,131 @@
> +/**
> + * 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/phy/ulpi/driver.h>
> +#include <linux/phy/ulpi/regs.h>
> +#include <linux/gpio/consumer.h>
> +
> +#include "ulpi_phy.h"
> +
> +struct tusb1210 {
> +	struct ulpi *ulpi;
> +	struct phy *phy;
> +	struct gpio_desc *gpio_reset;
> +	struct gpio_desc *gpio_cs;
> +	u8 ctx[1];
> +};
> +
> +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 optimisation value */
> +	ulpi_write(tusb->ulpi, ULPI_EXT_VENDOR_SPECIFIC, tusb->ctx[0]);
> +
> +	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, ULPI_EXT_VENDOR_SPECIFIC);

do they *all* use this register for eye diagram optimization or is this
something that Intel decided to do ?

(sorry, don't know much about tusb1210 other than it sucks like hell :-)

> +	if (ret < 0)
> +		return ret;
> +
> +	tusb->ctx[0] = 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);
> +	dev_set_drvdata(&ulpi->dev, tusb);
> +	return 0;
> +}
> +
> +static void tusb1210_remove(struct ulpi *ulpi)
> +{
> +	struct tusb1210 *tusb = dev_get_drvdata(&ulpi->dev);

completely unrelated to $subject, but we might want to have a
ulpi_{set,get}_drvdata() at some point.

In fact, we might decide to add an entire ULPI bus, eventually, though
I'm still considering if there's any benefit to that.

> +
> +	ulpi_phy_destroy(ulpi, tusb->phy);
> +}
> +
> +#define TI_VENDOR_ID 0x0451
> +
> +static 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");

comment says GPL 2 only, this says GPL 2+

-- 
balbi

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

Re: [PATCH 3/3] phy: ulpi: add driver for TI TUSB1210

[Heikki Krogerus]

On Tue, Jan 20, 2015 at 09:45:39AM -0600, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Jan 20, 2015 at 11:18:22AM +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>
> > ---
> >  drivers/phy/ulpi/Kconfig    |  11 ++++
> >  drivers/phy/ulpi/Makefile   |   2 +
> >  drivers/phy/ulpi/tusb1210.c | 131 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 144 insertions(+)
> >  create mode 100644 drivers/phy/ulpi/tusb1210.c
> > 
> > diff --git a/drivers/phy/ulpi/Kconfig b/drivers/phy/ulpi/Kconfig
> > index 8007df2..7cd6f82 100644
> > --- a/drivers/phy/ulpi/Kconfig
> > +++ b/drivers/phy/ulpi/Kconfig
> > @@ -7,3 +7,14 @@ config ULPI_PHY
> >  	  Say yes if you have ULPI PHY attached to your USB controller.
> >  
> >  	  If unsure, say N.
> > +
> > +if ULPI_PHY
> > +
> > +config ULPI_TUSB1210
> > +	tristate "TI TUSB1210 USB PHY module"
> > +	depends on POWER_SUPPLY
> > +	select USB_PHY
> > +	help
> > +	  Support for TI TUSB1210 USB ULPI PHY.
> > +
> > +endif
> > diff --git a/drivers/phy/ulpi/Makefile b/drivers/phy/ulpi/Makefile
> > index 59e61cb..7ee6679 100644
> > --- a/drivers/phy/ulpi/Makefile
> > +++ b/drivers/phy/ulpi/Makefile
> > @@ -1,2 +1,4 @@
> >  ulpiphy-y			:= ulpi.o
> >  obj-$(CONFIG_ULPI_PHY)		+= ulpiphy.o
> > +
> > +obj-$(CONFIG_ULPI_TUSB1210)	+= tusb1210.o
> > diff --git a/drivers/phy/ulpi/tusb1210.c b/drivers/phy/ulpi/tusb1210.c
> > new file mode 100644
> > index 0000000..ac77f98
> > --- /dev/null
> > +++ b/drivers/phy/ulpi/tusb1210.c
> 
> do you really need this extra ulpi directory ?
> 
> I wonder if phy-tusb1210.c as a name would be enough.

IMO grouping the ULPI PHY drivers and other ULPI bus code into
separate folder from the start is the right thing to do.

> > @@ -0,0 +1,131 @@
> > +/**
> > + * 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/phy/ulpi/driver.h>
> > +#include <linux/phy/ulpi/regs.h>
> > +#include <linux/gpio/consumer.h>
> > +
> > +#include "ulpi_phy.h"
> > +
> > +struct tusb1210 {
> > +	struct ulpi *ulpi;
> > +	struct phy *phy;
> > +	struct gpio_desc *gpio_reset;
> > +	struct gpio_desc *gpio_cs;
> > +	u8 ctx[1];
> > +};
> > +
> > +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 optimisation value */
> > +	ulpi_write(tusb->ulpi, ULPI_EXT_VENDOR_SPECIFIC, tusb->ctx[0]);
> > +
> > +	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, ULPI_EXT_VENDOR_SPECIFIC);
> 
> do they *all* use this register for eye diagram optimization or is this
> something that Intel decided to do ?
> 
> (sorry, don't know much about tusb1210 other than it sucks like hell :-)

All I know that somebody needs to save the value. The ones using this
PHY who don't need to save it can most likely live without the driver.

> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	tusb->ctx[0] = 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);
> > +	dev_set_drvdata(&ulpi->dev, tusb);
> > +	return 0;
> > +}
> > +
> > +static void tusb1210_remove(struct ulpi *ulpi)
> > +{
> > +	struct tusb1210 *tusb = dev_get_drvdata(&ulpi->dev);
> 
> completely unrelated to $subject, but we might want to have a
> ulpi_{set,get}_drvdata() at some point.

Makes sense.

> In fact, we might decide to add an entire ULPI bus, eventually, though
> I'm still considering if there's any benefit to that.

I don't think I understand this comment? ULPI bus is what I'm
introducing in this set (the first patch in it)?

> > +
> > +	ulpi_phy_destroy(ulpi, tusb->phy);
> > +}
> > +
> > +#define TI_VENDOR_ID 0x0451
> > +
> > +static 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");
> 
> comment says GPL 2 only, this says GPL 2+

True. I'll fix it.


Thanks!

-- 
heikki

Re: [PATCH 3/3] phy: ulpi: add driver for TI TUSB1210

[Heikki Krogerus]

Hi,

On Wed, Jan 21, 2015 at 11:17:49AM +0200, Heikki Krogerus wrote:
> On Tue, Jan 20, 2015 at 09:45:39AM -0600, Felipe Balbi wrote:
> > > diff --git a/drivers/phy/ulpi/tusb1210.c b/drivers/phy/ulpi/tusb1210.c
> > > new file mode 100644
> > > index 0000000..ac77f98
> > > --- /dev/null
> > > +++ b/drivers/phy/ulpi/tusb1210.c
> > 
> > do you really need this extra ulpi directory ?
> > 
> > I wonder if phy-tusb1210.c as a name would be enough.
> 
> IMO grouping the ULPI PHY drivers and other ULPI bus code into
> separate folder from the start is the right thing to do.

A correction to this comment. I probable don't need this folder. Like
you said, phy-tusb1210.c should be enough..

<snip>

> > In fact, we might decide to add an entire ULPI bus, eventually, though
> > I'm still considering if there's any benefit to that.
> 
> I don't think I understand this comment? ULPI bus is what I'm
> introducing in this set (the first patch in it)?

..I talked with Alex about this :). So I think the bus belongs under
drivers/usb/core/ instead of driver/phy/. It's not really tied to the
Generic PHY framework in any way, but ULPI is of course USB specific.


Cheers,

-- 
heikki

Re: [PATCH 3/3] phy: ulpi: add driver for TI TUSB1210

[Felipe Balbi]

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

On Wed, Jan 21, 2015 at 01:39:58PM +0200, Heikki Krogerus wrote:
> Hi,
> 
> On Wed, Jan 21, 2015 at 11:17:49AM +0200, Heikki Krogerus wrote:
> > On Tue, Jan 20, 2015 at 09:45:39AM -0600, Felipe Balbi wrote:
> > > > diff --git a/drivers/phy/ulpi/tusb1210.c b/drivers/phy/ulpi/tusb1210.c
> > > > new file mode 100644
> > > > index 0000000..ac77f98
> > > > --- /dev/null
> > > > +++ b/drivers/phy/ulpi/tusb1210.c
> > > 
> > > do you really need this extra ulpi directory ?
> > > 
> > > I wonder if phy-tusb1210.c as a name would be enough.
> > 
> > IMO grouping the ULPI PHY drivers and other ULPI bus code into
> > separate folder from the start is the right thing to do.
> 
> A correction to this comment. I probable don't need this folder. Like
> you said, phy-tusb1210.c should be enough..
> 
> <snip>
> 
> > > In fact, we might decide to add an entire ULPI bus, eventually, though
> > > I'm still considering if there's any benefit to that.
> > 
> > I don't think I understand this comment? ULPI bus is what I'm
> > introducing in this set (the first patch in it)?
> 
> ..I talked with Alex about this :). So I think the bus belongs under
> drivers/usb/core/ instead of driver/phy/. It's not really tied to the
> Generic PHY framework in any way, but ULPI is of course USB specific.

right, maybe drivers/usb/ulpi or maybe drivers/ulpi, and have
phy-tusb1201 register under that ulpi_bus_type instead of
platform_bus_type, but still use drivers/phy to register itself a phy
provider ;-)

-- 
balbi

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

Re: [PATCH 3/3] phy: ulpi: add driver for TI TUSB1210

[Heikki Krogerus]

On Thu, Jan 22, 2015 at 02:51:24PM -0600, Felipe Balbi wrote:
> On Wed, Jan 21, 2015 at 01:39:58PM +0200, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Wed, Jan 21, 2015 at 11:17:49AM +0200, Heikki Krogerus wrote:
> > > On Tue, Jan 20, 2015 at 09:45:39AM -0600, Felipe Balbi wrote:
> > > > > diff --git a/drivers/phy/ulpi/tusb1210.c b/drivers/phy/ulpi/tusb1210.c
> > > > > new file mode 100644
> > > > > index 0000000..ac77f98
> > > > > --- /dev/null
> > > > > +++ b/drivers/phy/ulpi/tusb1210.c
> > > > 
> > > > do you really need this extra ulpi directory ?
> > > > 
> > > > I wonder if phy-tusb1210.c as a name would be enough.
> > > 
> > > IMO grouping the ULPI PHY drivers and other ULPI bus code into
> > > separate folder from the start is the right thing to do.
> > 
> > A correction to this comment. I probable don't need this folder. Like
> > you said, phy-tusb1210.c should be enough..
> > 
> > <snip>
> > 
> > > > In fact, we might decide to add an entire ULPI bus, eventually, though
> > > > I'm still considering if there's any benefit to that.
> > > 
> > > I don't think I understand this comment? ULPI bus is what I'm
> > > introducing in this set (the first patch in it)?
> > 
> > ..I talked with Alex about this :). So I think the bus belongs under
> > drivers/usb/core/ instead of driver/phy/. It's not really tied to the
> > Generic PHY framework in any way, but ULPI is of course USB specific.
> 
> right, maybe drivers/usb/ulpi or maybe drivers/ulpi, and have
> phy-tusb1201 register under that ulpi_bus_type instead of
> platform_bus_type, but still use drivers/phy to register itself a phy
> provider ;-)

So just for the record: This driver does not register under
platform_bus_type bus but instead already under ulpi_bus_type.

I'll prepare new version out of these today and try to figure out
proper place for the code (maybe drivers/usb/ulpi?).


Cheers,

-- 
heikki

Re: [PATCH 3/3] phy: ulpi: add driver for TI TUSB1210

[Felipe Balbi]

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

On Fri, Jan 23, 2015 at 10:23:48AM +0200, Heikki Krogerus wrote:
> On Thu, Jan 22, 2015 at 02:51:24PM -0600, Felipe Balbi wrote:
> > On Wed, Jan 21, 2015 at 01:39:58PM +0200, Heikki Krogerus wrote:
> > > Hi,
> > > 
> > > On Wed, Jan 21, 2015 at 11:17:49AM +0200, Heikki Krogerus wrote:
> > > > On Tue, Jan 20, 2015 at 09:45:39AM -0600, Felipe Balbi wrote:
> > > > > > diff --git a/drivers/phy/ulpi/tusb1210.c b/drivers/phy/ulpi/tusb1210.c
> > > > > > new file mode 100644
> > > > > > index 0000000..ac77f98
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/phy/ulpi/tusb1210.c
> > > > > 
> > > > > do you really need this extra ulpi directory ?
> > > > > 
> > > > > I wonder if phy-tusb1210.c as a name would be enough.
> > > > 
> > > > IMO grouping the ULPI PHY drivers and other ULPI bus code into
> > > > separate folder from the start is the right thing to do.
> > > 
> > > A correction to this comment. I probable don't need this folder. Like
> > > you said, phy-tusb1210.c should be enough..
> > > 
> > > <snip>
> > > 
> > > > > In fact, we might decide to add an entire ULPI bus, eventually, though
> > > > > I'm still considering if there's any benefit to that.
> > > > 
> > > > I don't think I understand this comment? ULPI bus is what I'm
> > > > introducing in this set (the first patch in it)?
> > > 
> > > ..I talked with Alex about this :). So I think the bus belongs under
> > > drivers/usb/core/ instead of driver/phy/. It's not really tied to the
> > > Generic PHY framework in any way, but ULPI is of course USB specific.
> > 
> > right, maybe drivers/usb/ulpi or maybe drivers/ulpi, and have
> > phy-tusb1201 register under that ulpi_bus_type instead of
> > platform_bus_type, but still use drivers/phy to register itself a phy
> > provider ;-)
> 
> So just for the record: This driver does not register under
> platform_bus_type bus but instead already under ulpi_bus_type.

hah! :-)

> I'll prepare new version out of these today and try to figure out
> proper place for the code (maybe drivers/usb/ulpi?).

drivers/phy is fine, it is a phy driver anyway... I just should've read
it to see there was a ulpi_bus_type :-)

-- 
balbi

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

Re: [PATCH 3/3] phy: ulpi: add driver for TI TUSB1210

[Felipe Balbi]

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

Hi,

On Wed, Jan 21, 2015 at 11:17:49AM +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>
> > > ---
> > >  drivers/phy/ulpi/Kconfig    |  11 ++++
> > >  drivers/phy/ulpi/Makefile   |   2 +
> > >  drivers/phy/ulpi/tusb1210.c | 131 ++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 144 insertions(+)
> > >  create mode 100644 drivers/phy/ulpi/tusb1210.c
> > > 
> > > diff --git a/drivers/phy/ulpi/Kconfig b/drivers/phy/ulpi/Kconfig
> > > index 8007df2..7cd6f82 100644
> > > --- a/drivers/phy/ulpi/Kconfig
> > > +++ b/drivers/phy/ulpi/Kconfig
> > > @@ -7,3 +7,14 @@ config ULPI_PHY
> > >  	  Say yes if you have ULPI PHY attached to your USB controller.
> > >  
> > >  	  If unsure, say N.
> > > +
> > > +if ULPI_PHY
> > > +
> > > +config ULPI_TUSB1210
> > > +	tristate "TI TUSB1210 USB PHY module"
> > > +	depends on POWER_SUPPLY
> > > +	select USB_PHY
> > > +	help
> > > +	  Support for TI TUSB1210 USB ULPI PHY.
> > > +
> > > +endif
> > > diff --git a/drivers/phy/ulpi/Makefile b/drivers/phy/ulpi/Makefile
> > > index 59e61cb..7ee6679 100644
> > > --- a/drivers/phy/ulpi/Makefile
> > > +++ b/drivers/phy/ulpi/Makefile
> > > @@ -1,2 +1,4 @@
> > >  ulpiphy-y			:= ulpi.o
> > >  obj-$(CONFIG_ULPI_PHY)		+= ulpiphy.o
> > > +
> > > +obj-$(CONFIG_ULPI_TUSB1210)	+= tusb1210.o
> > > diff --git a/drivers/phy/ulpi/tusb1210.c b/drivers/phy/ulpi/tusb1210.c
> > > new file mode 100644
> > > index 0000000..ac77f98
> > > --- /dev/null
> > > +++ b/drivers/phy/ulpi/tusb1210.c
> > 
> > do you really need this extra ulpi directory ?
> > 
> > I wonder if phy-tusb1210.c as a name would be enough.
> 
> IMO grouping the ULPI PHY drivers and other ULPI bus code into
> separate folder from the start is the right thing to do.

because... :-)

> > > @@ -0,0 +1,131 @@
> > > +/**
> > > + * 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/phy/ulpi/driver.h>
> > > +#include <linux/phy/ulpi/regs.h>
> > > +#include <linux/gpio/consumer.h>
> > > +
> > > +#include "ulpi_phy.h"
> > > +
> > > +struct tusb1210 {
> > > +	struct ulpi *ulpi;
> > > +	struct phy *phy;
> > > +	struct gpio_desc *gpio_reset;
> > > +	struct gpio_desc *gpio_cs;
> > > +	u8 ctx[1];
> > > +};
> > > +
> > > +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 optimisation value */
> > > +	ulpi_write(tusb->ulpi, ULPI_EXT_VENDOR_SPECIFIC, tusb->ctx[0]);
> > > +
> > > +	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, ULPI_EXT_VENDOR_SPECIFIC);
> > 
> > do they *all* use this register for eye diagram optimization or is this
> > something that Intel decided to do ?
> > 
> > (sorry, don't know much about tusb1210 other than it sucks like hell :-)
> 
> All I know that somebody needs to save the value. The ones using this
> PHY who don't need to save it can most likely live without the driver.

right, but what I mean is: is it mandatory that Eye diagram
configuration be stored in *this* register? Or is it more like a scratch
register which Intel just happens to be using for Eye diagram data ?

> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	tusb->ctx[0] = 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);
> > > +	dev_set_drvdata(&ulpi->dev, tusb);
> > > +	return 0;
> > > +}
> > > +
> > > +static void tusb1210_remove(struct ulpi *ulpi)
> > > +{
> > > +	struct tusb1210 *tusb = dev_get_drvdata(&ulpi->dev);
> > 
> > completely unrelated to $subject, but we might want to have a
> > ulpi_{set,get}_drvdata() at some point.
> 
> Makes sense.
> 
> > In fact, we might decide to add an entire ULPI bus, eventually, though
> > I'm still considering if there's any benefit to that.
> 
> I don't think I understand this comment? ULPI bus is what I'm
> introducing in this set (the first patch in it)?

I mean introducing a real struct bus ulpi_bus_type :-) With match,
probe, remove, etc.

-- 
balbi

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

Re: [PATCH 3/3] phy: ulpi: add driver for TI TUSB1210

[Heikki Krogerus]

Hi,

On Thu, Jan 22, 2015 at 02:49:25PM -0600, Felipe Balbi wrote:
> On Wed, Jan 21, 2015 at 11:17:49AM +0200, Heikki Krogerus wrote:
> > > > +	/* Store initial eye diagram optimisation value */
> > > > +	ret = ulpi_read(ulpi, ULPI_EXT_VENDOR_SPECIFIC);
> > > 
> > > do they *all* use this register for eye diagram optimization or is this
> > > something that Intel decided to do ?
> > > 
> > > (sorry, don't know much about tusb1210 other than it sucks like hell :-)
> > 
> > All I know that somebody needs to save the value. The ones using this
> > PHY who don't need to save it can most likely live without the driver.
> 
> right, but what I mean is: is it mandatory that Eye diagram
> configuration be stored in *this* register? Or is it more like a scratch
> register which Intel just happens to be using for Eye diagram data ?

The eye diagram tuning is in that register. Here's the spec:
http://www.ti.com/lit/ds/symlink/tusb1210.pdf

I'll add definition for the register (which is colourfully named
"VENDOR_SPECIFIC2").

> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	tusb->ctx[0] = 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);
> > > > +	dev_set_drvdata(&ulpi->dev, tusb);
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void tusb1210_remove(struct ulpi *ulpi)
> > > > +{
> > > > +	struct tusb1210 *tusb = dev_get_drvdata(&ulpi->dev);
> > > 
> > > completely unrelated to $subject, but we might want to have a
> > > ulpi_{set,get}_drvdata() at some point.
> > 
> > Makes sense.
> > 
> > > In fact, we might decide to add an entire ULPI bus, eventually, though
> > > I'm still considering if there's any benefit to that.
> > 
> > I don't think I understand this comment? ULPI bus is what I'm
> > introducing in this set (the first patch in it)?
> 
> I mean introducing a real struct bus ulpi_bus_type :-) With match,
> probe, remove, etc.

I'm already doing that. Please check the first patch in this set:
"phy: add bus for USB ULPI PHYs".


Cheers,

-- 
heikki

Re: [PATCH 3/3] phy: ulpi: add driver for TI TUSB1210

[Felipe Balbi]

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

On Fri, Jan 23, 2015 at 10:12:41AM +0200, Heikki Krogerus wrote:
> Hi,
> 
> On Thu, Jan 22, 2015 at 02:49:25PM -0600, Felipe Balbi wrote:
> > On Wed, Jan 21, 2015 at 11:17:49AM +0200, Heikki Krogerus wrote:
> > > > > +	/* Store initial eye diagram optimisation value */
> > > > > +	ret = ulpi_read(ulpi, ULPI_EXT_VENDOR_SPECIFIC);
> > > > 
> > > > do they *all* use this register for eye diagram optimization or is this
> > > > something that Intel decided to do ?
> > > > 
> > > > (sorry, don't know much about tusb1210 other than it sucks like hell :-)
> > > 
> > > All I know that somebody needs to save the value. The ones using this
> > > PHY who don't need to save it can most likely live without the driver.
> > 
> > right, but what I mean is: is it mandatory that Eye diagram
> > configuration be stored in *this* register? Or is it more like a scratch
> > register which Intel just happens to be using for Eye diagram data ?
> 
> The eye diagram tuning is in that register. Here's the spec:
> http://www.ti.com/lit/ds/symlink/tusb1210.pdf
> 
> I'll add definition for the register (which is colourfully named
> "VENDOR_SPECIFIC2").

alright, thanks.

> > > > > +	if (ret < 0)
> > > > > +		return ret;
> > > > > +
> > > > > +	tusb->ctx[0] = 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);
> > > > > +	dev_set_drvdata(&ulpi->dev, tusb);
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static void tusb1210_remove(struct ulpi *ulpi)
> > > > > +{
> > > > > +	struct tusb1210 *tusb = dev_get_drvdata(&ulpi->dev);
> > > > 
> > > > completely unrelated to $subject, but we might want to have a
> > > > ulpi_{set,get}_drvdata() at some point.
> > > 
> > > Makes sense.
> > > 
> > > > In fact, we might decide to add an entire ULPI bus, eventually, though
> > > > I'm still considering if there's any benefit to that.
> > > 
> > > I don't think I understand this comment? ULPI bus is what I'm
> > > introducing in this set (the first patch in it)?
> > 
> > I mean introducing a real struct bus ulpi_bus_type :-) With match,
> > probe, remove, etc.
> 
> I'm already doing that. Please check the first patch in this set:
> "phy: add bus for USB ULPI PHYs".

yeah, sorry about that.

-- 
balbi

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