[PATCH v2 0/7] Make dwc3 use Generic PHY Framework

[PATCH v2 0/7] Make dwc3 use Generic PHY Framework

[Kishon Vijay Abraham I]

Felipe,

Looks like most of the patches are dependent on Generic PHY Framework except
the first one. Let me know if I have to take these patches with your ACK or
you'll take it yourself.
******************************************************************
Modified dwc3 core to find PHYs only if the platform indicates that it has 
to use a PHY. Adapted DWC3 and USB3 PHY to use Generic PHY framework. Also 
changed the name of USB3 PHY driver to PIPE3 PHY driver since the same driver 
has to be used for SATA and PCIE too.

Changes from v1:
* The logic in which the driver detects the presence of PHYs has changed.
* patch ordering has changed
* udelay is replaced with usleep_range
* A patch to remove set_suspend callback which was deferred from Generic
PHY Framework series has been included.

Kishon Vijay Abraham I (7):
  usb: dwc3: get "usb_phy" only if the platform indicates the presence
    of PHY's
  usb: dwc3: adapt dwc3 core to use Generic PHY Framework
  Documentation: dt bindings: move ..usb/usb-phy.txt to
    ..phy/ti-phy.txt
  drivers: phy: usb3/pipe3: Adapt pipe3 driver to Generic PHY Framework
  usb: phy: omap-usb2: remove *set_suspend* callback from omap-usb2
  phy: omap-usb2: move omap_usb.h from linux/usb/ to linux/phy/
  arm/dts: added dt properties to adapt to the new phy framwork

 .../bindings/{usb/usb-phy.txt => phy/ti-phy.txt}   |    9 +-
 Documentation/devicetree/bindings/usb/dwc3.txt     |    6 +-
 arch/arm/boot/dts/omap5.dtsi                       |    5 +-
 drivers/phy/Kconfig                                |   10 +
 drivers/phy/Makefile                               |    1 +
 drivers/phy/phy-omap-usb2.c                        |   27 +--
 .../phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c}     |  206 +++++++++++---------
 drivers/usb/dwc3/Kconfig                           |    2 +
 drivers/usb/dwc3/core.c                            |  124 ++++++++----
 drivers/usb/dwc3/core.h                            |    7 +
 drivers/usb/dwc3/platform_data.h                   |    4 +
 drivers/usb/phy/Kconfig                            |   11 --
 drivers/usb/phy/Makefile                           |    1 -
 include/linux/{usb => phy}/omap_usb.h              |    3 -
 include/linux/{usb/omap_usb.h => phy/ti_pipe3.h}   |   33 +---
 15 files changed, 248 insertions(+), 201 deletions(-)
 rename Documentation/devicetree/bindings/{usb/usb-phy.txt => phy/ti-phy.txt} (86%)
 rename drivers/{usb/phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c} (57%)
 copy include/linux/{usb => phy}/omap_usb.h (95%)
 rename include/linux/{usb/omap_usb.h => phy/ti_pipe3.h} (53%)

-- 
1.7.10.4

[PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Kishon Vijay Abraham I]

There can be systems which does not have a external usb_phy, so get
usb_phy only if dt data indicates the presence of PHY in the case of dt boot or
if platform_data indicates the presence of PHY. Also remove checking if
return value is -ENXIO since it's now changed to always enable usb_phy layer.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
In usb_get_phy_by_phandle, index 0 always refers to usb2 phy and index 1 always
refers to usb3 phy. Since we've lived so long with this, this patch will make
an assumption that if only one entry is populated in *usb-phy* property, it will
be usb2 phy and the next entry will be usb3 phy.

 drivers/usb/dwc3/Kconfig         |    1 +
 drivers/usb/dwc3/core.c          |   72 ++++++++++++++++++++------------------
 drivers/usb/dwc3/platform_data.h |    2 ++
 3 files changed, 41 insertions(+), 34 deletions(-)

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 70fc430..8e385b4 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -1,6 +1,7 @@
 config USB_DWC3
 	tristate "DesignWare USB3 DRD Core Support"
 	depends on (USB || USB_GADGET) && HAS_DMA
+	select USB_PHY
 	select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
 	help
 	  Say Y or M here if your system has a Dual Role SuperSpeed
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 474162e..cb91d70 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -354,6 +354,7 @@ static int dwc3_probe(struct platform_device *pdev)
 	struct device_node	*node = dev->of_node;
 	struct resource		*res;
 	struct dwc3		*dwc;
+	int			count;
 
 	int			ret = -ENOMEM;
 
@@ -387,16 +388,49 @@ static int dwc3_probe(struct platform_device *pdev)
 	if (node) {
 		dwc->maximum_speed = of_usb_get_maximum_speed(node);
 
-		dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
-		dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
+		count = of_count_phandle_with_args(node, "usb-phy", NULL);
+		switch (count) {
+		case 2:
+			dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev,
+				"usb-phy", 1);
+			if (IS_ERR(dwc->usb3_phy)) {
+				dev_err(dev, "usb3 phy not found\n");
+				return PTR_ERR(dwc->usb3_phy);
+			}
+		case 1:
+			dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev,
+				"usb-phy", 0);
+			if (IS_ERR(dwc->usb2_phy)) {
+				dev_err(dev, "usb2 phy not found\n");
+				return PTR_ERR(dwc->usb2_phy);
+			}
+			break;
+		default:
+			dev_err(dev, "usb phy nodes not configured\n");
+		}
 
 		dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize");
 		dwc->dr_mode = of_usb_get_dr_mode(node);
 	} else if (pdata) {
 		dwc->maximum_speed = pdata->maximum_speed;
 
-		dwc->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
-		dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
+		if (pdata->usb2_phy) {
+			dwc->usb2_phy = devm_usb_get_phy(dev,
+				USB_PHY_TYPE_USB2);
+			if (IS_ERR(dwc->usb2_phy)) {
+				dev_err(dev, "usb2 phy not found\n");
+				return PTR_ERR(dwc->usb2_phy);
+			}
+		}
+
+		if (pdata->usb3_phy) {
+			dwc->usb3_phy = devm_usb_get_phy(dev,
+				USB_PHY_TYPE_USB3);
+			if (IS_ERR(dwc->usb3_phy)) {
+				dev_err(dev, "usb3 phy not found\n");
+				return PTR_ERR(dwc->usb3_phy);
+			}
+		}
 
 		dwc->needs_fifo_resize = pdata->tx_fifo_resize;
 		dwc->dr_mode = pdata->dr_mode;
@@ -409,36 +443,6 @@ static int dwc3_probe(struct platform_device *pdev)
 	if (dwc->maximum_speed == USB_SPEED_UNKNOWN)
 		dwc->maximum_speed = USB_SPEED_SUPER;
 
-	if (IS_ERR(dwc->usb2_phy)) {
-		ret = PTR_ERR(dwc->usb2_phy);
-
-		/*
-		 * if -ENXIO is returned, it means PHY layer wasn't
-		 * enabled, so it makes no sense to return -EPROBE_DEFER
-		 * in that case, since no PHY driver will ever probe.
-		 */
-		if (ret == -ENXIO)
-			return ret;
-
-		dev_err(dev, "no usb2 phy configured\n");
-		return -EPROBE_DEFER;
-	}
-
-	if (IS_ERR(dwc->usb3_phy)) {
-		ret = PTR_ERR(dwc->usb3_phy);
-
-		/*
-		 * if -ENXIO is returned, it means PHY layer wasn't
-		 * enabled, so it makes no sense to return -EPROBE_DEFER
-		 * in that case, since no PHY driver will ever probe.
-		 */
-		if (ret == -ENXIO)
-			return ret;
-
-		dev_err(dev, "no usb3 phy configured\n");
-		return -EPROBE_DEFER;
-	}
-
 	dwc->xhci_resources[0].start = res->start;
 	dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
 					DWC3_XHCI_REGS_END;
diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h
index 7db34f0..49ffa6d 100644
--- a/drivers/usb/dwc3/platform_data.h
+++ b/drivers/usb/dwc3/platform_data.h
@@ -24,4 +24,6 @@ struct dwc3_platform_data {
 	enum usb_device_speed maximum_speed;
 	enum usb_dr_mode dr_mode;
 	bool tx_fifo_resize;
+	bool usb2_phy;
+	bool usb3_phy;
 };
-- 
1.7.10.4

[PATCH v2 2/7] usb: dwc3: adapt dwc3 core to use Generic PHY Framework

[Kishon Vijay Abraham I]

Adapted dwc3 core to use the Generic PHY Framework. So for init, exit,
power_on and power_off the following APIs are used phy_init(), phy_exit(),
phy_power_on() and phy_power_off().

However using the old USB phy library wont be removed till the PHYs of all
other SoC's using dwc3 core is adapted to the Generic PHY Framework.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 Documentation/devicetree/bindings/usb/dwc3.txt |    6 ++-
 drivers/usb/dwc3/Kconfig                       |    1 +
 drivers/usb/dwc3/core.c                        |   52 ++++++++++++++++++++++++
 drivers/usb/dwc3/core.h                        |    7 ++++
 drivers/usb/dwc3/platform_data.h               |    2 +
 5 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index e807635..471366d 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -6,11 +6,13 @@ Required properties:
  - compatible: must be "snps,dwc3"
  - reg : Address and length of the register set for the device
  - interrupts: Interrupts used by the dwc3 controller.
+
+Optional properties:
  - usb-phy : array of phandle for the PHY device.  The first element
    in the array is expected to be a handle to the USB2/HS PHY and
    the second element is expected to be a handle to the USB3/SS PHY
-
-Optional properties:
+ - phys: from the *Generic PHY* bindings
+ - phy-names: from the *Generic PHY* bindings
  - tx-fifo-resize: determines if the FIFO *has* to be reallocated.
 
 This is usually a subnode to DWC3 glue to which it is connected.
diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 8e385b4..64eed6f 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -2,6 +2,7 @@ config USB_DWC3
 	tristate "DesignWare USB3 DRD Core Support"
 	depends on (USB || USB_GADGET) && HAS_DMA
 	select USB_PHY
+	select GENERIC_PHY
 	select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
 	help
 	  Say Y or M here if your system has a Dual Role SuperSpeed
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index cb91d70..28bfa5b 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -82,6 +82,12 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
 
 	usb_phy_init(dwc->usb2_phy);
 	usb_phy_init(dwc->usb3_phy);
+
+	if (dwc->usb2_generic_phy)
+		phy_init(dwc->usb2_generic_phy);
+	if (dwc->usb3_generic_phy)
+		phy_init(dwc->usb3_generic_phy);
+
 	mdelay(100);
 
 	/* Clear USB3 PHY reset */
@@ -343,6 +349,11 @@ static void dwc3_core_exit(struct dwc3 *dwc)
 {
 	usb_phy_shutdown(dwc->usb2_phy);
 	usb_phy_shutdown(dwc->usb3_phy);
+
+	if (dwc->usb2_generic_phy)
+		phy_power_off(dwc->usb2_generic_phy);
+	if (dwc->usb3_generic_phy)
+		phy_power_off(dwc->usb3_generic_phy);
 }
 
 #define DWC3_ALIGN_MASK		(16 - 1)
@@ -439,6 +450,26 @@ static int dwc3_probe(struct platform_device *pdev)
 		dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
 	}
 
+	count = of_property_match_string(node, "phy-names", "usb2-phy");
+	if (count >= 0 || (pdata && pdata->usb2_generic_phy)) {
+		dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
+		if (IS_ERR(dwc->usb2_generic_phy)) {
+			dev_err(dev, "no usb2 phy configured yet");
+			return PTR_ERR(dwc->usb2_generic_phy);
+		}
+		dwc->usb2_phy = NULL;
+	}
+
+	count = of_property_match_string(node, "phy-names", "usb3-phy");
+	if (count >= 0 || (pdata && pdata->usb3_generic_phy)) {
+		dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
+		if (IS_ERR(dwc->usb3_generic_phy)) {
+			dev_err(dev, "no usb3 phy configured yet");
+			return PTR_ERR(dwc->usb3_generic_phy);
+		}
+		dwc->usb3_phy = NULL;
+	}
+
 	/* default to superspeed if no maximum_speed passed */
 	if (dwc->maximum_speed == USB_SPEED_UNKNOWN)
 		dwc->maximum_speed = USB_SPEED_SUPER;
@@ -462,6 +493,11 @@ static int dwc3_probe(struct platform_device *pdev)
 	usb_phy_set_suspend(dwc->usb2_phy, 0);
 	usb_phy_set_suspend(dwc->usb3_phy, 0);
 
+	if (dwc->usb2_generic_phy)
+		phy_power_on(dwc->usb2_generic_phy);
+	if (dwc->usb3_generic_phy)
+		phy_power_on(dwc->usb3_generic_phy);
+
 	spin_lock_init(&dwc->lock);
 	platform_set_drvdata(pdev, dwc);
 
@@ -588,6 +624,11 @@ static int dwc3_remove(struct platform_device *pdev)
 	usb_phy_set_suspend(dwc->usb2_phy, 1);
 	usb_phy_set_suspend(dwc->usb3_phy, 1);
 
+	if (dwc->usb2_generic_phy)
+		phy_power_off(dwc->usb2_generic_phy);
+	if (dwc->usb3_generic_phy)
+		phy_power_off(dwc->usb3_generic_phy);
+
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
@@ -685,6 +726,11 @@ static int dwc3_suspend(struct device *dev)
 	usb_phy_shutdown(dwc->usb3_phy);
 	usb_phy_shutdown(dwc->usb2_phy);
 
+	if (dwc->usb2_generic_phy)
+		phy_exit(dwc->usb2_generic_phy);
+	if (dwc->usb3_generic_phy)
+		phy_exit(dwc->usb3_generic_phy);
+
 	return 0;
 }
 
@@ -695,6 +741,12 @@ static int dwc3_resume(struct device *dev)
 
 	usb_phy_init(dwc->usb3_phy);
 	usb_phy_init(dwc->usb2_phy);
+
+	if (dwc->usb2_generic_phy)
+		phy_init(dwc->usb2_generic_phy);
+	if (dwc->usb3_generic_phy)
+		phy_init(dwc->usb3_generic_phy);
+
 	msleep(100);
 
 	spin_lock_irqsave(&dwc->lock, flags);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index f8af8d4..01ec7d7 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -31,6 +31,8 @@
 #include <linux/usb/gadget.h>
 #include <linux/usb/otg.h>
 
+#include <linux/phy/phy.h>
+
 /* Global constants */
 #define DWC3_EP0_BOUNCE_SIZE	512
 #define DWC3_ENDPOINTS_NUM	32
@@ -613,6 +615,8 @@ struct dwc3_scratchpad_array {
  * @dr_mode: requested mode of operation
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
+ * @usb2_generic_phy: pointer to USB2 PHY
+ * @usb3_generic_phy: pointer to USB3 PHY
  * @dcfg: saved contents of DCFG register
  * @gctl: saved contents of GCTL register
  * @is_selfpowered: true when we are selfpowered
@@ -665,6 +669,9 @@ struct dwc3 {
 	struct usb_phy		*usb2_phy;
 	struct usb_phy		*usb3_phy;
 
+	struct phy		*usb2_generic_phy;
+	struct phy		*usb3_generic_phy;
+
 	void __iomem		*regs;
 	size_t			regs_size;
 
diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h
index 49ffa6d..fc62a0f 100644
--- a/drivers/usb/dwc3/platform_data.h
+++ b/drivers/usb/dwc3/platform_data.h
@@ -26,4 +26,6 @@ struct dwc3_platform_data {
 	bool tx_fifo_resize;
 	bool usb2_phy;
 	bool usb3_phy;
+	bool usb2_generic_phy;
+	bool usb3_generic_phy;
 };
-- 
1.7.10.4

[PATCH v2 3/7] Documentation: dt bindings: move ..usb/usb-phy.txt to ..phy/ti-phy.txt

[Kishon Vijay Abraham I]

Since now we have a separate folder for phy, move the PHY dt binding
documentation of TI to that folder.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../devicetree/bindings/{usb/usb-phy.txt => phy/ti-phy.txt}   |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
 rename Documentation/devicetree/bindings/{usb/usb-phy.txt => phy/ti-phy.txt} (86%)

diff --git a/Documentation/devicetree/bindings/usb/usb-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
similarity index 86%
rename from Documentation/devicetree/bindings/usb/usb-phy.txt
rename to Documentation/devicetree/bindings/phy/ti-phy.txt
index c0245c8..355acf0 100644
--- a/Documentation/devicetree/bindings/usb/usb-phy.txt
+++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
@@ -1,4 +1,4 @@
-USB PHY
+TI PHY: DT DOCUMENTATION FOR PHYs in TI PLATFORMs
 
 OMAP USB2 PHY
 
@@ -21,10 +21,11 @@ usb2phy@4a0ad080 {
 	#phy-cells = <0>;
 };
 
-OMAP USB3 PHY
+TI PIPE3 PHY
 
 Required properties:
- - compatible: Should be "ti,omap-usb3"
+ - compatible: Should be "ti,phy-usb3", "ti,phy-pcie" or "ti,phy-sata".
+   "ti,omap-usb3" is deprecated.
  - reg : Address and length of the register set for the device.
  - reg-names: The names of the register addresses corresponding to the registers
    filled in "reg".
@@ -38,7 +39,7 @@ Optional properties:
 This is usually a subnode of ocp2scp to which it is connected.
 
 usb3phy@4a084400 {
-	compatible = "ti,omap-usb3";
+	compatible = "ti,phy-usb3";
 	reg = <0x4a084400 0x80>,
 	      <0x4a084800 0x64>,
 	      <0x4a084c00 0x40>;
-- 
1.7.10.4

[PATCH v2 4/7] drivers: phy: usb3/pipe3: Adapt pipe3 driver to Generic PHY Framework

[Kishon Vijay Abraham I]

Adapted omap-usb3 PHY driver to Generic PHY Framework and moved phy-omap-usb3
driver in drivers/usb/phy to drivers/phy and also renamed the file to
phy-ti-pipe3 since this same driver will be used for SATA PHY and
PCIE PHY.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/Kconfig                                |   10 +
 drivers/phy/Makefile                               |    1 +
 .../phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c}     |  206 +++++++++++---------
 drivers/usb/phy/Kconfig                            |   11 --
 drivers/usb/phy/Makefile                           |    1 -
 include/linux/phy/ti_pipe3.h                       |   52 +++++
 6 files changed, 174 insertions(+), 107 deletions(-)
 rename drivers/{usb/phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c} (57%)
 create mode 100644 include/linux/phy/ti_pipe3.h

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index ac239ac..1158943 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -27,6 +27,16 @@ config OMAP_USB2
 	  The USB OTG controller communicates with the comparator using this
 	  driver.
 
+config TI_PIPE3
+	tristate "TI PIPE3 PHY Driver"
+	select GENERIC_PHY
+	select OMAP_CONTROL_USB
+	help
+	  Enable this to support the PIPE3 PHY that is part of TI SOCs. This
+	  driver takes care of all the PHY functionality apart from comparator.
+	  This driver interacts with the "OMAP Control PHY Driver" to power
+	  on/off the PHY.
+
 config TWL4030_USB
 	tristate "TWL4030 USB Transceiver Driver"
 	depends on TWL4030_CORE && REGULATOR_TWL4030 && USB_MUSB_OMAP2PLUS
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 0dd8a98..42ccab4 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -4,4 +4,5 @@
 
 obj-$(CONFIG_GENERIC_PHY)	+= phy-core.o
 obj-$(CONFIG_OMAP_USB2)		+= phy-omap-usb2.o
+obj-$(CONFIG_TI_PIPE3)		+= phy-ti-pipe3.o
 obj-$(CONFIG_TWL4030_USB)	+= phy-twl4030-usb.o
diff --git a/drivers/usb/phy/phy-omap-usb3.c b/drivers/phy/phy-ti-pipe3.c
similarity index 57%
rename from drivers/usb/phy/phy-omap-usb3.c
rename to drivers/phy/phy-ti-pipe3.c
index 0c6ba29..31e8397 100644
--- a/drivers/usb/phy/phy-omap-usb3.c
+++ b/drivers/phy/phy-ti-pipe3.c
@@ -1,5 +1,5 @@
 /*
- * omap-usb3 - USB PHY, talking to dwc3 controller in OMAP.
+ * phy-ti-pipe3 - PIPE3 PHY driver for SATA, USB and PCIE.
  *
  * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
  * This program is free software; you can redistribute it and/or modify
@@ -19,7 +19,8 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
-#include <linux/usb/omap_usb.h>
+#include <linux/phy/ti_pipe3.h>
+#include <linux/phy/phy.h>
 #include <linux/of.h>
 #include <linux/clk.h>
 #include <linux/err.h>
@@ -52,17 +53,17 @@
 
 /*
  * This is an Empirical value that works, need to confirm the actual
- * value required for the USB3PHY_PLL_CONFIGURATION2.PLL_IDLE status
- * to be correctly reflected in the USB3PHY_PLL_STATUS register.
+ * value required for the PIPE3PHY_PLL_CONFIGURATION2.PLL_IDLE status
+ * to be correctly reflected in the PIPE3PHY_PLL_STATUS register.
  */
 # define PLL_IDLE_TIME  100;
 
-struct usb_dpll_map {
+struct pipe3_dpll_map {
 	unsigned long rate;
-	struct usb_dpll_params params;
+	struct pipe3_dpll_params params;
 };
 
-static struct usb_dpll_map dpll_map[] = {
+static struct pipe3_dpll_map dpll_map[] = {
 	{12000000, {1250, 5, 4, 20, 0} },	/* 12 MHz */
 	{16800000, {3125, 20, 4, 20, 0} },	/* 16.8 MHz */
 	{19200000, {1172, 8, 4, 20, 65537} },	/* 19.2 MHz */
@@ -71,7 +72,7 @@ static struct usb_dpll_map dpll_map[] = {
 	{38400000, {3125, 47, 4, 20, 92843} },	/* 38.4 MHz */
 };
 
-static struct usb_dpll_params *omap_usb3_get_dpll_params(unsigned long rate)
+static struct pipe3_dpll_params *ti_pipe3_get_dpll_params(unsigned long rate)
 {
 	int i;
 
@@ -83,110 +84,113 @@ static struct usb_dpll_params *omap_usb3_get_dpll_params(unsigned long rate)
 	return NULL;
 }
 
-static int omap_usb3_suspend(struct usb_phy *x, int suspend)
+static int ti_pipe3_power_off(struct phy *x)
 {
-	struct omap_usb *phy = phy_to_omapusb(x);
-	int	val;
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+	int val;
 	int timeout = PLL_IDLE_TIME;
 
-	if (suspend && !phy->is_suspended) {
-		val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
-		val |= PLL_IDLE;
-		omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
-
-		do {
-			val = omap_usb_readl(phy->pll_ctrl_base, PLL_STATUS);
-			if (val & PLL_TICOPWDN)
-				break;
-			udelay(1);
-		} while (--timeout);
-
-		omap_control_usb_phy_power(phy->control_dev, 0);
-
-		phy->is_suspended	= 1;
-	} else if (!suspend && phy->is_suspended) {
-		phy->is_suspended	= 0;
-
-		val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
-		val &= ~PLL_IDLE;
-		omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
-
-		do {
-			val = omap_usb_readl(phy->pll_ctrl_base, PLL_STATUS);
-			if (!(val & PLL_TICOPWDN))
-				break;
-			udelay(1);
-		} while (--timeout);
-	}
+	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
+	val |= PLL_IDLE;
+	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
+
+	do {
+		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
+		if (val & PLL_TICOPWDN)
+			break;
+		usleep_range(1, 5);
+	} while (--timeout);
+
+	omap_control_usb_phy_power(phy->control_dev, 0);
+
+	return 0;
+}
+
+static int ti_pipe3_power_on(struct phy *x)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+	int val;
+	int timeout = PLL_IDLE_TIME;
+
+	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
+	val &= ~PLL_IDLE;
+	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
+
+	do {
+		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
+		if (!(val & PLL_TICOPWDN))
+			break;
+		usleep_range(1, 5);
+	} while (--timeout);
 
 	return 0;
 }
 
-static void omap_usb_dpll_relock(struct omap_usb *phy)
+static void ti_pipe3_dpll_relock(struct ti_pipe3 *phy)
 {
 	u32		val;
 	unsigned long	timeout;
 
-	omap_usb_writel(phy->pll_ctrl_base, PLL_GO, SET_PLL_GO);
+	ti_pipe3_writel(phy->pll_ctrl_base, PLL_GO, SET_PLL_GO);
 
 	timeout = jiffies + msecs_to_jiffies(20);
 	do {
-		val = omap_usb_readl(phy->pll_ctrl_base, PLL_STATUS);
+		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
 		if (val & PLL_LOCK)
 			break;
 	} while (!WARN_ON(time_after(jiffies, timeout)));
 }
 
-static int omap_usb_dpll_lock(struct omap_usb *phy)
+static int ti_pipe3_dpll_lock(struct ti_pipe3 *phy)
 {
 	u32			val;
 	unsigned long		rate;
-	struct usb_dpll_params *dpll_params;
+	struct pipe3_dpll_params *dpll_params;
 
 	rate = clk_get_rate(phy->sys_clk);
-	dpll_params = omap_usb3_get_dpll_params(rate);
+	dpll_params = ti_pipe3_get_dpll_params(rate);
 	if (!dpll_params) {
 		dev_err(phy->dev,
 			  "No DPLL configuration for %lu Hz SYS CLK\n", rate);
 		return -EINVAL;
 	}
 
-	val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
+	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
 	val &= ~PLL_REGN_MASK;
 	val |= dpll_params->n << PLL_REGN_SHIFT;
-	omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
+	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
 
-	val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
+	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 	val &= ~PLL_SELFREQDCO_MASK;
 	val |= dpll_params->freq << PLL_SELFREQDCO_SHIFT;
-	omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
+	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
 
-	val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
+	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
 	val &= ~PLL_REGM_MASK;
 	val |= dpll_params->m << PLL_REGM_SHIFT;
-	omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
+	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
 
-	val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION4);
+	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION4);
 	val &= ~PLL_REGM_F_MASK;
 	val |= dpll_params->mf << PLL_REGM_F_SHIFT;
-	omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION4, val);
+	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION4, val);
 
-	val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION3);
+	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION3);
 	val &= ~PLL_SD_MASK;
 	val |= dpll_params->sd << PLL_SD_SHIFT;
-	omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION3, val);
+	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION3, val);
 
-	omap_usb_dpll_relock(phy);
+	ti_pipe3_dpll_relock(phy);
 
 	return 0;
 }
 
-static int omap_usb3_init(struct usb_phy *x)
+static int ti_pipe3_init(struct phy *x)
 {
-	struct omap_usb	*phy = phy_to_omapusb(x);
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
 	int ret;
 
-	ret = omap_usb_dpll_lock(phy);
+	ret = ti_pipe3_dpll_lock(phy);
 	if (ret)
 		return ret;
 
@@ -195,9 +199,18 @@ static int omap_usb3_init(struct usb_phy *x)
 	return 0;
 }
 
-static int omap_usb3_probe(struct platform_device *pdev)
+static struct phy_ops ops = {
+	.init		= ti_pipe3_init,
+	.power_on	= ti_pipe3_power_on,
+	.power_off	= ti_pipe3_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int ti_pipe3_probe(struct platform_device *pdev)
 {
-	struct omap_usb *phy;
+	struct ti_pipe3 *phy;
+	struct phy *generic_phy;
+	struct phy_provider *phy_provider;
 	struct resource *res;
 	struct device_node *node = pdev->dev.of_node;
 	struct device_node *control_node;
@@ -208,7 +221,7 @@ static int omap_usb3_probe(struct platform_device *pdev)
 
 	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
 	if (!phy) {
-		dev_err(&pdev->dev, "unable to alloc mem for OMAP USB3 PHY\n");
+		dev_err(&pdev->dev, "unable to alloc mem for TI PIPE3 PHY\n");
 		return -ENOMEM;
 	}
 
@@ -219,13 +232,6 @@ static int omap_usb3_probe(struct platform_device *pdev)
 
 	phy->dev		= &pdev->dev;
 
-	phy->phy.dev		= phy->dev;
-	phy->phy.label		= "omap-usb3";
-	phy->phy.init		= omap_usb3_init;
-	phy->phy.set_suspend	= omap_usb3_suspend;
-	phy->phy.type		= USB_PHY_TYPE_USB3;
-
-	phy->is_suspended	= 1;
 	phy->wkupclk = devm_clk_get(phy->dev, "usb_phy_cm_clk32k");
 	if (IS_ERR(phy->wkupclk)) {
 		dev_err(&pdev->dev, "unable to get usb_phy_cm_clk32k\n");
@@ -251,6 +257,11 @@ static int omap_usb3_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Failed to get control device phandle\n");
 		return -EINVAL;
 	}
+	phy_provider = devm_of_phy_provider_register(phy->dev,
+			of_phy_simple_xlate);
+	if (IS_ERR(phy_provider))
+		return PTR_ERR(phy_provider);
+
 	control_pdev = of_find_device_by_node(control_node);
 	if (!control_pdev) {
 		dev_err(&pdev->dev, "Failed to get control device\n");
@@ -260,23 +271,27 @@ static int omap_usb3_probe(struct platform_device *pdev)
 	phy->control_dev = &control_pdev->dev;
 
 	omap_control_usb_phy_power(phy->control_dev, 0);
-	usb_add_phy_dev(&phy->phy);
 
 	platform_set_drvdata(pdev, phy);
-
 	pm_runtime_enable(phy->dev);
+
+	generic_phy = devm_phy_create(phy->dev, &ops, NULL);
+	if (IS_ERR(generic_phy))
+		return PTR_ERR(generic_phy);
+
+	phy_set_drvdata(generic_phy, phy);
+
 	pm_runtime_get(&pdev->dev);
 
 	return 0;
 }
 
-static int omap_usb3_remove(struct platform_device *pdev)
+static int ti_pipe3_remove(struct platform_device *pdev)
 {
-	struct omap_usb *phy = platform_get_drvdata(pdev);
+	struct ti_pipe3 *phy = platform_get_drvdata(pdev);
 
 	clk_unprepare(phy->wkupclk);
 	clk_unprepare(phy->optclk);
-	usb_remove_phy(&phy->phy);
 	if (!pm_runtime_suspended(&pdev->dev))
 		pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
@@ -286,10 +301,9 @@ static int omap_usb3_remove(struct platform_device *pdev)
 
 #ifdef CONFIG_PM_RUNTIME
 
-static int omap_usb3_runtime_suspend(struct device *dev)
+static int ti_pipe3_runtime_suspend(struct device *dev)
 {
-	struct platform_device	*pdev = to_platform_device(dev);
-	struct omap_usb	*phy = platform_get_drvdata(pdev);
+	struct ti_pipe3	*phy = dev_get_drvdata(dev);
 
 	clk_disable(phy->wkupclk);
 	clk_disable(phy->optclk);
@@ -297,11 +311,10 @@ static int omap_usb3_runtime_suspend(struct device *dev)
 	return 0;
 }
 
-static int omap_usb3_runtime_resume(struct device *dev)
+static int ti_pipe3_runtime_resume(struct device *dev)
 {
 	u32 ret = 0;
-	struct platform_device	*pdev = to_platform_device(dev);
-	struct omap_usb	*phy = platform_get_drvdata(pdev);
+	struct ti_pipe3	*phy = dev_get_drvdata(dev);
 
 	ret = clk_enable(phy->optclk);
 	if (ret) {
@@ -324,38 +337,41 @@ err1:
 	return ret;
 }
 
-static const struct dev_pm_ops omap_usb3_pm_ops = {
-	SET_RUNTIME_PM_OPS(omap_usb3_runtime_suspend, omap_usb3_runtime_resume,
-		NULL)
+static const struct dev_pm_ops ti_pipe3_pm_ops = {
+	SET_RUNTIME_PM_OPS(ti_pipe3_runtime_suspend,
+		ti_pipe3_runtime_resume, NULL)
 };
 
-#define DEV_PM_OPS     (&omap_usb3_pm_ops)
+#define DEV_PM_OPS     (&ti_pipe3_pm_ops)
 #else
 #define DEV_PM_OPS     NULL
 #endif
 
 #ifdef CONFIG_OF
-static const struct of_device_id omap_usb3_id_table[] = {
+static const struct of_device_id ti_pipe3_id_table[] = {
+	{ .compatible = "ti,phy-sata" },
+	{ .compatible = "ti,phy-pcie" },
+	{ .compatible = "ti,phy-usb3" },
 	{ .compatible = "ti,omap-usb3" },
 	{}
 };
-MODULE_DEVICE_TABLE(of, omap_usb3_id_table);
+MODULE_DEVICE_TABLE(of, ti_pipe3_id_table);
 #endif
 
-static struct platform_driver omap_usb3_driver = {
-	.probe		= omap_usb3_probe,
-	.remove		= omap_usb3_remove,
+static struct platform_driver ti_pipe3_driver = {
+	.probe		= ti_pipe3_probe,
+	.remove		= ti_pipe3_remove,
 	.driver		= {
-		.name	= "omap-usb3",
+		.name	= "ti-pipe3",
 		.owner	= THIS_MODULE,
 		.pm	= DEV_PM_OPS,
-		.of_match_table = of_match_ptr(omap_usb3_id_table),
+		.of_match_table = of_match_ptr(ti_pipe3_id_table),
 	},
 };
 
-module_platform_driver(omap_usb3_driver);
+module_platform_driver(ti_pipe3_driver);
 
-MODULE_ALIAS("platform: omap_usb3");
+MODULE_ALIAS("platform: ti_pipe3");
 MODULE_AUTHOR("Texas Instruments Inc.");
-MODULE_DESCRIPTION("OMAP USB3 phy driver");
+MODULE_DESCRIPTION("TI PIPE3 phy driver");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
index 64b8bef..1d4916a 100644
--- a/drivers/usb/phy/Kconfig
+++ b/drivers/usb/phy/Kconfig
@@ -66,17 +66,6 @@ config OMAP_CONTROL_USB
 	  power on the USB2 PHY is present in OMAP4 and OMAP5. OMAP5 has an
 	  additional register to power on USB3 PHY.
 
-config OMAP_USB3
-	tristate "OMAP USB3 PHY Driver"
-	depends on ARCH_OMAP2PLUS || COMPILE_TEST
-	select OMAP_CONTROL_USB
-	select USB_PHY
-	help
-	  Enable this to support the USB3 PHY that is part of SOC. This
-	  driver takes care of all the PHY functionality apart from comparator.
-	  This driver interacts with the "OMAP Control USB Driver" to power
-	  on/off the PHY.
-
 config AM335X_CONTROL_USB
 	tristate
 
diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
index 9c37361..fa80661 100644
--- a/drivers/usb/phy/Makefile
+++ b/drivers/usb/phy/Makefile
@@ -15,7 +15,6 @@ obj-$(CONFIG_NOP_USB_XCEIV)		+= phy-generic.o
 obj-$(CONFIG_OMAP_CONTROL_USB)		+= phy-omap-control.o
 obj-$(CONFIG_AM335X_CONTROL_USB)	+= phy-am335x-control.o
 obj-$(CONFIG_AM335X_PHY_USB)		+= phy-am335x.o
-obj-$(CONFIG_OMAP_USB3)			+= phy-omap-usb3.o
 obj-$(CONFIG_SAMSUNG_USBPHY)		+= phy-samsung-usb.o
 obj-$(CONFIG_SAMSUNG_USB2PHY)		+= phy-samsung-usb2.o
 obj-$(CONFIG_SAMSUNG_USB3PHY)		+= phy-samsung-usb3.o
diff --git a/include/linux/phy/ti_pipe3.h b/include/linux/phy/ti_pipe3.h
new file mode 100644
index 0000000..4d42b04
--- /dev/null
+++ b/include/linux/phy/ti_pipe3.h
@@ -0,0 +1,52 @@
+/*
+ * ti_pipe3.h -- ti pipe3 phy header file
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * Author: Kishon Vijay Abraham I <kishon@ti.com>
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __DRIVERS_TI_PIPE3_H
+#define __DRIVERS_TI_PIPE3_H
+
+#include <linux/io.h>
+
+struct pipe3_dpll_params {
+	u16	m;
+	u8	n;
+	u8	freq:3;
+	u8	sd;
+	u32	mf;
+};
+
+struct ti_pipe3 {
+	void __iomem		*pll_ctrl_base;
+	struct device		*dev;
+	struct device		*control_dev;
+	struct clk		*wkupclk;
+	struct clk		*sys_clk;
+	struct clk		*optclk;
+};
+
+static inline u32 ti_pipe3_readl(void __iomem *addr, unsigned offset)
+{
+	return __raw_readl(addr + offset);
+}
+
+static inline void ti_pipe3_writel(void __iomem *addr, unsigned offset,
+	u32 data)
+{
+	__raw_writel(data, addr + offset);
+}
+
+#endif /* __DRIVERS_TI_PIPE3_H */
-- 
1.7.10.4

[PATCH v2 5/7] usb: phy: omap-usb2: remove *set_suspend* callback from omap-usb2

[Kishon Vijay Abraham I]

Now that omap-usb2 is adapted to the new generic PHY framework,
*set_suspend* ops can be removed from omap-usb2 driver.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Acked-by: Felipe Balbi <balbi@ti.com>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
This patch was deferred from Generic PHY framework series since dwc3 uses the
same driver and it has to be adapted to the Generic PHY Framework.

 drivers/phy/phy-omap-usb2.c |   25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/drivers/phy/phy-omap-usb2.c b/drivers/phy/phy-omap-usb2.c
index bfc5c33..9b3e867 100644
--- a/drivers/phy/phy-omap-usb2.c
+++ b/drivers/phy/phy-omap-usb2.c
@@ -98,28 +98,6 @@ static int omap_usb_set_peripheral(struct usb_otg *otg,
 	return 0;
 }
 
-static int omap_usb2_suspend(struct usb_phy *x, int suspend)
-{
-	struct omap_usb *phy = phy_to_omapusb(x);
-	int ret;
-
-	if (suspend && !phy->is_suspended) {
-		omap_control_usb_phy_power(phy->control_dev, 0);
-		pm_runtime_put_sync(phy->dev);
-		phy->is_suspended = 1;
-	} else if (!suspend && phy->is_suspended) {
-		ret = pm_runtime_get_sync(phy->dev);
-		if (ret < 0) {
-			dev_err(phy->dev, "get_sync failed with err %d\n", ret);
-			return ret;
-		}
-		omap_control_usb_phy_power(phy->control_dev, 1);
-		phy->is_suspended = 0;
-	}
-
-	return 0;
-}
-
 static int omap_usb_power_off(struct phy *x)
 {
 	struct omap_usb *phy = phy_get_drvdata(x);
@@ -173,7 +151,6 @@ static int omap_usb2_probe(struct platform_device *pdev)
 
 	phy->phy.dev		= phy->dev;
 	phy->phy.label		= "omap-usb2";
-	phy->phy.set_suspend	= omap_usb2_suspend;
 	phy->phy.otg		= otg;
 	phy->phy.type		= USB_PHY_TYPE_USB2;
 
@@ -195,8 +172,6 @@ static int omap_usb2_probe(struct platform_device *pdev)
 	}
 
 	phy->control_dev = &control_pdev->dev;
-
-	phy->is_suspended	= 1;
 	omap_control_usb_phy_power(phy->control_dev, 0);
 
 	otg->set_host		= omap_usb_set_host;
-- 
1.7.10.4

[PATCH v2 6/7] phy: omap-usb2: move omap_usb.h from linux/usb/ to linux/phy/

[Kishon Vijay Abraham I]

No functional change. Moved omap_usb.h from linux/usb/ to linux/phy/.
Also removed the unused members of struct omap_usb (after phy-omap-pipe3
started using it's own header file)

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/phy-omap-usb2.c           |    2 +-
 include/linux/{usb => phy}/omap_usb.h |    3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)
 rename include/linux/{usb => phy}/omap_usb.h (95%)

diff --git a/drivers/phy/phy-omap-usb2.c b/drivers/phy/phy-omap-usb2.c
index 9b3e867..d738dc3 100644
--- a/drivers/phy/phy-omap-usb2.c
+++ b/drivers/phy/phy-omap-usb2.c
@@ -21,7 +21,7 @@
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/io.h>
-#include <linux/usb/omap_usb.h>
+#include <linux/phy/omap_usb.h>
 #include <linux/usb/phy_companion.h>
 #include <linux/clk.h>
 #include <linux/err.h>
diff --git a/include/linux/usb/omap_usb.h b/include/linux/phy/omap_usb.h
similarity index 95%
rename from include/linux/usb/omap_usb.h
rename to include/linux/phy/omap_usb.h
index 6ae2936..19d343c3 100644
--- a/include/linux/usb/omap_usb.h
+++ b/include/linux/phy/omap_usb.h
@@ -33,13 +33,10 @@ struct usb_dpll_params {
 struct omap_usb {
 	struct usb_phy		phy;
 	struct phy_companion	*comparator;
-	void __iomem		*pll_ctrl_base;
 	struct device		*dev;
 	struct device		*control_dev;
 	struct clk		*wkupclk;
-	struct clk		*sys_clk;
 	struct clk		*optclk;
-	u8			is_suspended:1;
 };
 
 #define	phy_to_omapusb(x)	container_of((x), struct omap_usb, phy)
-- 
1.7.10.4

[PATCH v2 7/7] arm/dts: added dt properties to adapt to the new phy framwork

[Kishon Vijay Abraham I]

Added device tree bindings for dwc3, usb2 and usb3 PHYs. The documentation
of these can be found at Documentation/devicetree/bindings/phy/phy-bindings.txt
and Documentation/devicetree/bindings/phy/ti-phy.txt.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 arch/arm/boot/dts/omap5.dtsi |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/omap5.dtsi b/arch/arm/boot/dts/omap5.dtsi
index edc801f..998f198 100644
--- a/arch/arm/boot/dts/omap5.dtsi
+++ b/arch/arm/boot/dts/omap5.dtsi
@@ -661,7 +661,8 @@
 				compatible = "snps,dwc3";
 				reg = <0x4a030000 0x10000>;
 				interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH>;
-				usb-phy = <&usb2_phy>, <&usb3_phy>;
+				phys = <&usb2_phy>, <&usb3_phy>;
+				phy-names = "usb2-phy", "usb3-phy";
 				tx-fifo-resize;
 			};
 		};
@@ -677,6 +678,7 @@
 				compatible = "ti,omap-usb2";
 				reg = <0x4a084000 0x7c>;
 				ctrl-module = <&omap_control_usb2phy>;
+				#phy-cells = <0>;
 			};
 
 			usb3_phy: usb3phy@4a084400 {
@@ -686,6 +688,7 @@
 				      <0x4a084c00 0x40>;
 				reg-names = "phy_rx", "phy_tx", "pll_ctrl";
 				ctrl-module = <&omap_control_usb3phy>;
+				#phy-cells = <0>;
 			};
 		};
 
-- 
1.7.10.4

Re: [PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Roger Quadros]

Hi Kishon,

On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote:
> There can be systems which does not have a external usb_phy, so get
> usb_phy only if dt data indicates the presence of PHY in the case of dt boot or
> if platform_data indicates the presence of PHY. Also remove checking if
> return value is -ENXIO since it's now changed to always enable usb_phy layer.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
> In usb_get_phy_by_phandle, index 0 always refers to usb2 phy and index 1 always
> refers to usb3 phy. Since we've lived so long with this, this patch will make
> an assumption that if only one entry is populated in *usb-phy* property, it will
> be usb2 phy and the next entry will be usb3 phy.
> 
>  drivers/usb/dwc3/Kconfig         |    1 +
>  drivers/usb/dwc3/core.c          |   72 ++++++++++++++++++++------------------
>  drivers/usb/dwc3/platform_data.h |    2 ++
>  3 files changed, 41 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index 70fc430..8e385b4 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -1,6 +1,7 @@
>  config USB_DWC3
>  	tristate "DesignWare USB3 DRD Core Support"
>  	depends on (USB || USB_GADGET) && HAS_DMA
> +	select USB_PHY
>  	select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
>  	help
>  	  Say Y or M here if your system has a Dual Role SuperSpeed
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 474162e..cb91d70 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -354,6 +354,7 @@ static int dwc3_probe(struct platform_device *pdev)
>  	struct device_node	*node = dev->of_node;
>  	struct resource		*res;
>  	struct dwc3		*dwc;
> +	int			count;
>  
>  	int			ret = -ENOMEM;
>  
> @@ -387,16 +388,49 @@ static int dwc3_probe(struct platform_device *pdev)
>  	if (node) {
>  		dwc->maximum_speed = of_usb_get_maximum_speed(node);
>  
> -		dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
> -		dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
> +		count = of_count_phandle_with_args(node, "usb-phy", NULL);
> +		switch (count) {
> +		case 2:
> +			dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev,
> +				"usb-phy", 1);
> +			if (IS_ERR(dwc->usb3_phy)) {
> +				dev_err(dev, "usb3 phy not found\n");
> +				return PTR_ERR(dwc->usb3_phy);
> +			}
> +		case 1:
> +			dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev,
> +				"usb-phy", 0);
> +			if (IS_ERR(dwc->usb2_phy)) {
> +				dev_err(dev, "usb2 phy not found\n");
> +				return PTR_ERR(dwc->usb2_phy);
> +			}
> +			break;

In the Exynos case, there is only 1 phy and it is the USB3 phy. This code
will wrongly treat it as usb2_phy.


> +		default:
> +			dev_err(dev, "usb phy nodes not configured\n");
> +		}
>  
>  		dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize");
>  		dwc->dr_mode = of_usb_get_dr_mode(node);
>  	} else if (pdata) {
>  		dwc->maximum_speed = pdata->maximum_speed;
>  
> -		dwc->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> -		dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
> +		if (pdata->usb2_phy) {
> +			dwc->usb2_phy = devm_usb_get_phy(dev,
> +				USB_PHY_TYPE_USB2);
> +			if (IS_ERR(dwc->usb2_phy)) {
> +				dev_err(dev, "usb2 phy not found\n");
> +				return PTR_ERR(dwc->usb2_phy);
> +			}
> +		}
> +
> +		if (pdata->usb3_phy) {
> +			dwc->usb3_phy = devm_usb_get_phy(dev,
> +				USB_PHY_TYPE_USB3);
> +			if (IS_ERR(dwc->usb3_phy)) {
> +				dev_err(dev, "usb3 phy not found\n");
> +				return PTR_ERR(dwc->usb3_phy);
> +			}
> +		}

This part looks fine to me.
>  
>  		dwc->needs_fifo_resize = pdata->tx_fifo_resize;
>  		dwc->dr_mode = pdata->dr_mode;
> @@ -409,36 +443,6 @@ static int dwc3_probe(struct platform_device *pdev)
>  	if (dwc->maximum_speed == USB_SPEED_UNKNOWN)
>  		dwc->maximum_speed = USB_SPEED_SUPER;
>  
> -	if (IS_ERR(dwc->usb2_phy)) {
> -		ret = PTR_ERR(dwc->usb2_phy);
> -
> -		/*
> -		 * if -ENXIO is returned, it means PHY layer wasn't
> -		 * enabled, so it makes no sense to return -EPROBE_DEFER
> -		 * in that case, since no PHY driver will ever probe.
> -		 */
> -		if (ret == -ENXIO)
> -			return ret;
> -
> -		dev_err(dev, "no usb2 phy configured\n");
> -		return -EPROBE_DEFER;
> -	}
> -
> -	if (IS_ERR(dwc->usb3_phy)) {
> -		ret = PTR_ERR(dwc->usb3_phy);
> -
> -		/*
> -		 * if -ENXIO is returned, it means PHY layer wasn't
> -		 * enabled, so it makes no sense to return -EPROBE_DEFER
> -		 * in that case, since no PHY driver will ever probe.
> -		 */
> -		if (ret == -ENXIO)
> -			return ret;
> -
> -		dev_err(dev, "no usb3 phy configured\n");
> -		return -EPROBE_DEFER;
> -	}
> -
>  	dwc->xhci_resources[0].start = res->start;
>  	dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
>  					DWC3_XHCI_REGS_END;
> diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h
> index 7db34f0..49ffa6d 100644
> --- a/drivers/usb/dwc3/platform_data.h
> +++ b/drivers/usb/dwc3/platform_data.h
> @@ -24,4 +24,6 @@ struct dwc3_platform_data {
>  	enum usb_device_speed maximum_speed;
>  	enum usb_dr_mode dr_mode;
>  	bool tx_fifo_resize;
> +	bool usb2_phy;
> +	bool usb3_phy;
>  };
> 


cheers,
-roger

Re: [PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Kishon Vijay Abraham I]

Hi roger,

On Wednesday 16 October 2013 06:33 PM, Roger Quadros wrote:
> Hi Kishon,
> 
> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote:
>> There can be systems which does not have a external usb_phy, so get
>> usb_phy only if dt data indicates the presence of PHY in the case of dt boot or
>> if platform_data indicates the presence of PHY. Also remove checking if
>> return value is -ENXIO since it's now changed to always enable usb_phy layer.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>> In usb_get_phy_by_phandle, index 0 always refers to usb2 phy and index 1 always
>> refers to usb3 phy. Since we've lived so long with this, this patch will make
>> an assumption that if only one entry is populated in *usb-phy* property, it will
>> be usb2 phy and the next entry will be usb3 phy.
>>
>>  drivers/usb/dwc3/Kconfig         |    1 +
>>  drivers/usb/dwc3/core.c          |   72 ++++++++++++++++++++------------------
>>  drivers/usb/dwc3/platform_data.h |    2 ++
>>  3 files changed, 41 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>> index 70fc430..8e385b4 100644
>> --- a/drivers/usb/dwc3/Kconfig
>> +++ b/drivers/usb/dwc3/Kconfig
>> @@ -1,6 +1,7 @@
>>  config USB_DWC3
>>  	tristate "DesignWare USB3 DRD Core Support"
>>  	depends on (USB || USB_GADGET) && HAS_DMA
>> +	select USB_PHY
>>  	select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
>>  	help
>>  	  Say Y or M here if your system has a Dual Role SuperSpeed
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 474162e..cb91d70 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -354,6 +354,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>  	struct device_node	*node = dev->of_node;
>>  	struct resource		*res;
>>  	struct dwc3		*dwc;
>> +	int			count;
>>  
>>  	int			ret = -ENOMEM;
>>  
>> @@ -387,16 +388,49 @@ static int dwc3_probe(struct platform_device *pdev)
>>  	if (node) {
>>  		dwc->maximum_speed = of_usb_get_maximum_speed(node);
>>  
>> -		dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
>> -		dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
>> +		count = of_count_phandle_with_args(node, "usb-phy", NULL);
>> +		switch (count) {
>> +		case 2:
>> +			dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev,
>> +				"usb-phy", 1);
>> +			if (IS_ERR(dwc->usb3_phy)) {
>> +				dev_err(dev, "usb3 phy not found\n");
>> +				return PTR_ERR(dwc->usb3_phy);
>> +			}
>> +		case 1:
>> +			dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev,
>> +				"usb-phy", 0);
>> +			if (IS_ERR(dwc->usb2_phy)) {
>> +				dev_err(dev, "usb2 phy not found\n");
>> +				return PTR_ERR(dwc->usb2_phy);
>> +			}
>> +			break;
> 
> In the Exynos case, there is only 1 phy and it is the USB3 phy. This code
> will wrongly treat it as usb2_phy.

That was the case even before this patch no? Unfortunately the old USB PHY
library doesn't have APIs to get PHYs in a better way. If we try modifying the
USB PHY library, it'll be kind of duplicating what is already there in the
Generic PHY library. I'd rather prefer Exynos guys to use the new framework.

Thanks
Kishon

Re: [PATCH v2 2/7] usb: dwc3: adapt dwc3 core to use Generic PHY Framework

[Roger Quadros]

Hi,

On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote:
> Adapted dwc3 core to use the Generic PHY Framework. So for init, exit,
> power_on and power_off the following APIs are used phy_init(), phy_exit(),
> phy_power_on() and phy_power_off().
> 
> However using the old USB phy library wont be removed till the PHYs of all
> other SoC's using dwc3 core is adapted to the Generic PHY Framework.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  Documentation/devicetree/bindings/usb/dwc3.txt |    6 ++-
>  drivers/usb/dwc3/Kconfig                       |    1 +
>  drivers/usb/dwc3/core.c                        |   52 ++++++++++++++++++++++++
>  drivers/usb/dwc3/core.h                        |    7 ++++
>  drivers/usb/dwc3/platform_data.h               |    2 +
>  5 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index e807635..471366d 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -6,11 +6,13 @@ Required properties:
>   - compatible: must be "snps,dwc3"
>   - reg : Address and length of the register set for the device
>   - interrupts: Interrupts used by the dwc3 controller.
> +
> +Optional properties:
>   - usb-phy : array of phandle for the PHY device.  The first element
>     in the array is expected to be a handle to the USB2/HS PHY and
>     the second element is expected to be a handle to the USB3/SS PHY
> -
> -Optional properties:
> + - phys: from the *Generic PHY* bindings
> + - phy-names: from the *Generic PHY* bindings
>   - tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>  
>  This is usually a subnode to DWC3 glue to which it is connected.
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index 8e385b4..64eed6f 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -2,6 +2,7 @@ config USB_DWC3
>  	tristate "DesignWare USB3 DRD Core Support"
>  	depends on (USB || USB_GADGET) && HAS_DMA
>  	select USB_PHY
> +	select GENERIC_PHY
>  	select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
>  	help
>  	  Say Y or M here if your system has a Dual Role SuperSpeed
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index cb91d70..28bfa5b 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -82,6 +82,12 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
>  
>  	usb_phy_init(dwc->usb2_phy);
>  	usb_phy_init(dwc->usb3_phy);
> +
> +	if (dwc->usb2_generic_phy)
> +		phy_init(dwc->usb2_generic_phy);
> +	if (dwc->usb3_generic_phy)
> +		phy_init(dwc->usb3_generic_phy);
> +

dwc3_core_soft_reset() is called from dwc3_core_init() which is called from
dwc3_probe() but before the PHYs are initialized.

This will cause phy power_on() to be called before phy_init() which is wrong.

>  	mdelay(100);
>  
>  	/* Clear USB3 PHY reset */
> @@ -343,6 +349,11 @@ static void dwc3_core_exit(struct dwc3 *dwc)
>  {
>  	usb_phy_shutdown(dwc->usb2_phy);
>  	usb_phy_shutdown(dwc->usb3_phy);
> +
> +	if (dwc->usb2_generic_phy)
> +		phy_power_off(dwc->usb2_generic_phy);
> +	if (dwc->usb3_generic_phy)
> +		phy_power_off(dwc->usb3_generic_phy);
>  }
>  
>  #define DWC3_ALIGN_MASK		(16 - 1)
> @@ -439,6 +450,26 @@ static int dwc3_probe(struct platform_device *pdev)
>  		dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
>  	}
>  
> +	count = of_property_match_string(node, "phy-names", "usb2-phy");
> +	if (count >= 0 || (pdata && pdata->usb2_generic_phy)) {
> +		dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> +		if (IS_ERR(dwc->usb2_generic_phy)) {
> +			dev_err(dev, "no usb2 phy configured yet");
> +			return PTR_ERR(dwc->usb2_generic_phy);
> +		}
> +		dwc->usb2_phy = NULL;
> +	}
> +
> +	count = of_property_match_string(node, "phy-names", "usb3-phy");
> +	if (count >= 0 || (pdata && pdata->usb3_generic_phy)) {
> +		dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> +		if (IS_ERR(dwc->usb3_generic_phy)) {
> +			dev_err(dev, "no usb3 phy configured yet");
> +			return PTR_ERR(dwc->usb3_generic_phy);
> +		}
> +		dwc->usb3_phy = NULL;
> +	}
> +

There are a couple of issues here.
- The above code must be executed only if it node is valid.
- We must either get both PHYs via old method or via new method and not support mix matching
them. e.g. it is possible with this code that usb2_phy is set and usb3_generic_phy is set.
If you give the new method preference then you don't even need to attempt a devm_usb_get_phy()
if the generic phy is present in the node.


>  	/* default to superspeed if no maximum_speed passed */
>  	if (dwc->maximum_speed == USB_SPEED_UNKNOWN)
>  		dwc->maximum_speed = USB_SPEED_SUPER;
> @@ -462,6 +493,11 @@ static int dwc3_probe(struct platform_device *pdev)
>  	usb_phy_set_suspend(dwc->usb2_phy, 0);
>  	usb_phy_set_suspend(dwc->usb3_phy, 0);
>  
> +	if (dwc->usb2_generic_phy)
> +		phy_power_on(dwc->usb2_generic_phy);
> +	if (dwc->usb3_generic_phy)
> +		phy_power_on(dwc->usb3_generic_phy);
> +
>  	spin_lock_init(&dwc->lock);
>  	platform_set_drvdata(pdev, dwc);
>  
> @@ -588,6 +624,11 @@ static int dwc3_remove(struct platform_device *pdev)
>  	usb_phy_set_suspend(dwc->usb2_phy, 1);
>  	usb_phy_set_suspend(dwc->usb3_phy, 1);
>  
> +	if (dwc->usb2_generic_phy)
> +		phy_power_off(dwc->usb2_generic_phy);
> +	if (dwc->usb3_generic_phy)
> +		phy_power_off(dwc->usb3_generic_phy);
> +
>  	pm_runtime_put(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
> @@ -685,6 +726,11 @@ static int dwc3_suspend(struct device *dev)
>  	usb_phy_shutdown(dwc->usb3_phy);
>  	usb_phy_shutdown(dwc->usb2_phy);
>  
> +	if (dwc->usb2_generic_phy)
> +		phy_exit(dwc->usb2_generic_phy);
> +	if (dwc->usb3_generic_phy)
> +		phy_exit(dwc->usb3_generic_phy);
> +
>  	return 0;
>  }
>  
> @@ -695,6 +741,12 @@ static int dwc3_resume(struct device *dev)
>  
>  	usb_phy_init(dwc->usb3_phy);
>  	usb_phy_init(dwc->usb2_phy);
> +
> +	if (dwc->usb2_generic_phy)
> +		phy_init(dwc->usb2_generic_phy);
> +	if (dwc->usb3_generic_phy)
> +		phy_init(dwc->usb3_generic_phy);
> +
>  	msleep(100);
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index f8af8d4..01ec7d7 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -31,6 +31,8 @@
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/otg.h>
>  
> +#include <linux/phy/phy.h>
> +
>  /* Global constants */
>  #define DWC3_EP0_BOUNCE_SIZE	512
>  #define DWC3_ENDPOINTS_NUM	32
> @@ -613,6 +615,8 @@ struct dwc3_scratchpad_array {
>   * @dr_mode: requested mode of operation
>   * @usb2_phy: pointer to USB2 PHY
>   * @usb3_phy: pointer to USB3 PHY
> + * @usb2_generic_phy: pointer to USB2 PHY
> + * @usb3_generic_phy: pointer to USB3 PHY
>   * @dcfg: saved contents of DCFG register
>   * @gctl: saved contents of GCTL register
>   * @is_selfpowered: true when we are selfpowered
> @@ -665,6 +669,9 @@ struct dwc3 {
>  	struct usb_phy		*usb2_phy;
>  	struct usb_phy		*usb3_phy;
>  
> +	struct phy		*usb2_generic_phy;
> +	struct phy		*usb3_generic_phy;
> +
>  	void __iomem		*regs;
>  	size_t			regs_size;
>  
> diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h
> index 49ffa6d..fc62a0f 100644
> --- a/drivers/usb/dwc3/platform_data.h
> +++ b/drivers/usb/dwc3/platform_data.h
> @@ -26,4 +26,6 @@ struct dwc3_platform_data {
>  	bool tx_fifo_resize;
>  	bool usb2_phy;
>  	bool usb3_phy;
> +	bool usb2_generic_phy;
> +	bool usb3_generic_phy;
>  };
> 

cheers,
-roger

Re: [PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Roger Quadros]

On 10/16/2013 04:10 PM, Kishon Vijay Abraham I wrote:
> Hi roger,
> 
> On Wednesday 16 October 2013 06:33 PM, Roger Quadros wrote:
>> Hi Kishon,
>>
>> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote:
>>> There can be systems which does not have a external usb_phy, so get
>>> usb_phy only if dt data indicates the presence of PHY in the case of dt boot or
>>> if platform_data indicates the presence of PHY. Also remove checking if
>>> return value is -ENXIO since it's now changed to always enable usb_phy layer.
>>>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>> In usb_get_phy_by_phandle, index 0 always refers to usb2 phy and index 1 always
>>> refers to usb3 phy. Since we've lived so long with this, this patch will make
>>> an assumption that if only one entry is populated in *usb-phy* property, it will
>>> be usb2 phy and the next entry will be usb3 phy.
>>>
>>>  drivers/usb/dwc3/Kconfig         |    1 +
>>>  drivers/usb/dwc3/core.c          |   72 ++++++++++++++++++++------------------
>>>  drivers/usb/dwc3/platform_data.h |    2 ++
>>>  3 files changed, 41 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>> index 70fc430..8e385b4 100644
>>> --- a/drivers/usb/dwc3/Kconfig
>>> +++ b/drivers/usb/dwc3/Kconfig
>>> @@ -1,6 +1,7 @@
>>>  config USB_DWC3
>>>  	tristate "DesignWare USB3 DRD Core Support"
>>>  	depends on (USB || USB_GADGET) && HAS_DMA
>>> +	select USB_PHY
>>>  	select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
>>>  	help
>>>  	  Say Y or M here if your system has a Dual Role SuperSpeed
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 474162e..cb91d70 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -354,6 +354,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>>  	struct device_node	*node = dev->of_node;
>>>  	struct resource		*res;
>>>  	struct dwc3		*dwc;
>>> +	int			count;
>>>  
>>>  	int			ret = -ENOMEM;
>>>  
>>> @@ -387,16 +388,49 @@ static int dwc3_probe(struct platform_device *pdev)
>>>  	if (node) {
>>>  		dwc->maximum_speed = of_usb_get_maximum_speed(node);
>>>  
>>> -		dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
>>> -		dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
>>> +		count = of_count_phandle_with_args(node, "usb-phy", NULL);
>>> +		switch (count) {
>>> +		case 2:
>>> +			dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev,
>>> +				"usb-phy", 1);
>>> +			if (IS_ERR(dwc->usb3_phy)) {
>>> +				dev_err(dev, "usb3 phy not found\n");
>>> +				return PTR_ERR(dwc->usb3_phy);
>>> +			}
>>> +		case 1:
>>> +			dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev,
>>> +				"usb-phy", 0);
>>> +			if (IS_ERR(dwc->usb2_phy)) {
>>> +				dev_err(dev, "usb2 phy not found\n");
>>> +				return PTR_ERR(dwc->usb2_phy);
>>> +			}
>>> +			break;
>>
>> In the Exynos case, there is only 1 phy and it is the USB3 phy. This code
>> will wrongly treat it as usb2_phy.
> 
> That was the case even before this patch no? Unfortunately the old USB PHY
> library doesn't have APIs to get PHYs in a better way. If we try modifying the
> USB PHY library, it'll be kind of duplicating what is already there in the
> Generic PHY library. I'd rather prefer Exynos guys to use the new framework.

OK. I agree with you.
Do you know if there are users of dwc3 other than exynos5250 and omap5?
If not, we should get rid of the old USB PHY altogether.

cheers,
-roger

Re: [PATCH v2 4/7] drivers: phy: usb3/pipe3: Adapt pipe3 driver to Generic PHY Framework

[Roger Quadros]

Hi,

On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote:
> Adapted omap-usb3 PHY driver to Generic PHY Framework and moved phy-omap-usb3
> driver in drivers/usb/phy to drivers/phy and also renamed the file to
> phy-ti-pipe3 since this same driver will be used for SATA PHY and
> PCIE PHY.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/phy/Kconfig                                |   10 +
>  drivers/phy/Makefile                               |    1 +
>  .../phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c}     |  206 +++++++++++---------
>  drivers/usb/phy/Kconfig                            |   11 --
>  drivers/usb/phy/Makefile                           |    1 -
>  include/linux/phy/ti_pipe3.h                       |   52 +++++
>  6 files changed, 174 insertions(+), 107 deletions(-)
>  rename drivers/{usb/phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c} (57%)
>  create mode 100644 include/linux/phy/ti_pipe3.h
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index ac239ac..1158943 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -27,6 +27,16 @@ config OMAP_USB2
>  	  The USB OTG controller communicates with the comparator using this
>  	  driver.
>  
> +config TI_PIPE3
> +	tristate "TI PIPE3 PHY Driver"
> +	select GENERIC_PHY
> +	select OMAP_CONTROL_USB

The original OMAP_USB3 config had this
	depends on ARCH_OMAP2PLUS || COMPILE_TEST

You need the 'depends on ARCH_OMAP2PLUS' else you'll risk build failure on non OMAP
platforms, because OMAP_CONTROL_USB depends on ARCH_OMAP2PLUS.

> +	help
> +	  Enable this to support the PIPE3 PHY that is part of TI SOCs. This
> +	  driver takes care of all the PHY functionality apart from comparator.
> +	  This driver interacts with the "OMAP Control PHY Driver" to power
> +	  on/off the PHY.
> +
>  config TWL4030_USB
>  	tristate "TWL4030 USB Transceiver Driver"
>  	depends on TWL4030_CORE && REGULATOR_TWL4030 && USB_MUSB_OMAP2PLUS
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 0dd8a98..42ccab4 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -4,4 +4,5 @@
>  
>  obj-$(CONFIG_GENERIC_PHY)	+= phy-core.o
>  obj-$(CONFIG_OMAP_USB2)		+= phy-omap-usb2.o
> +obj-$(CONFIG_TI_PIPE3)		+= phy-ti-pipe3.o
>  obj-$(CONFIG_TWL4030_USB)	+= phy-twl4030-usb.o
> diff --git a/drivers/usb/phy/phy-omap-usb3.c b/drivers/phy/phy-ti-pipe3.c
> similarity index 57%
> rename from drivers/usb/phy/phy-omap-usb3.c
> rename to drivers/phy/phy-ti-pipe3.c
> index 0c6ba29..31e8397 100644
> --- a/drivers/usb/phy/phy-omap-usb3.c
> +++ b/drivers/phy/phy-ti-pipe3.c
> @@ -1,5 +1,5 @@
>  /*
> - * omap-usb3 - USB PHY, talking to dwc3 controller in OMAP.
> + * phy-ti-pipe3 - PIPE3 PHY driver for SATA, USB and PCIE.

SATA and PCIe is not yet supported so no need to mention about it now.

>   *
>   * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
>   * This program is free software; you can redistribute it and/or modify
> @@ -19,7 +19,8 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> -#include <linux/usb/omap_usb.h>
> +#include <linux/phy/ti_pipe3.h>
> +#include <linux/phy/phy.h>
>  #include <linux/of.h>
>  #include <linux/clk.h>
>  #include <linux/err.h>
> @@ -52,17 +53,17 @@
>  
>  /*
>   * This is an Empirical value that works, need to confirm the actual
> - * value required for the USB3PHY_PLL_CONFIGURATION2.PLL_IDLE status
> - * to be correctly reflected in the USB3PHY_PLL_STATUS register.
> + * value required for the PIPE3PHY_PLL_CONFIGURATION2.PLL_IDLE status
> + * to be correctly reflected in the PIPE3PHY_PLL_STATUS register.
>   */
>  # define PLL_IDLE_TIME  100;
>  
> -struct usb_dpll_map {
> +struct pipe3_dpll_map {
>  	unsigned long rate;
> -	struct usb_dpll_params params;
> +	struct pipe3_dpll_params params;
>  };
>  
> -static struct usb_dpll_map dpll_map[] = {
> +static struct pipe3_dpll_map dpll_map[] = {
>  	{12000000, {1250, 5, 4, 20, 0} },	/* 12 MHz */
>  	{16800000, {3125, 20, 4, 20, 0} },	/* 16.8 MHz */
>  	{19200000, {1172, 8, 4, 20, 65537} },	/* 19.2 MHz */
> @@ -71,7 +72,7 @@ static struct usb_dpll_map dpll_map[] = {
>  	{38400000, {3125, 47, 4, 20, 92843} },	/* 38.4 MHz */
>  };
>  
> -static struct usb_dpll_params *omap_usb3_get_dpll_params(unsigned long rate)
> +static struct pipe3_dpll_params *ti_pipe3_get_dpll_params(unsigned long rate)
>  {
>  	int i;
>  
> @@ -83,110 +84,113 @@ static struct usb_dpll_params *omap_usb3_get_dpll_params(unsigned long rate)
>  	return NULL;
>  }
>  
> -static int omap_usb3_suspend(struct usb_phy *x, int suspend)
> +static int ti_pipe3_power_off(struct phy *x)
>  {
> -	struct omap_usb *phy = phy_to_omapusb(x);
> -	int	val;
> +	struct ti_pipe3 *phy = phy_get_drvdata(x);
> +	int val;
>  	int timeout = PLL_IDLE_TIME;
>  
> -	if (suspend && !phy->is_suspended) {
> -		val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
> -		val |= PLL_IDLE;
> -		omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
> -
> -		do {
> -			val = omap_usb_readl(phy->pll_ctrl_base, PLL_STATUS);
> -			if (val & PLL_TICOPWDN)
> -				break;
> -			udelay(1);
> -		} while (--timeout);
> -
> -		omap_control_usb_phy_power(phy->control_dev, 0);
> -
> -		phy->is_suspended	= 1;
> -	} else if (!suspend && phy->is_suspended) {
> -		phy->is_suspended	= 0;
> -
> -		val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
> -		val &= ~PLL_IDLE;
> -		omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
> -
> -		do {
> -			val = omap_usb_readl(phy->pll_ctrl_base, PLL_STATUS);
> -			if (!(val & PLL_TICOPWDN))
> -				break;
> -			udelay(1);
> -		} while (--timeout);
> -	}
> +	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
> +	val |= PLL_IDLE;
> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
> +
> +	do {
> +		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
> +		if (val & PLL_TICOPWDN)
> +			break;
> +		usleep_range(1, 5);
> +	} while (--timeout);
> +
> +	omap_control_usb_phy_power(phy->control_dev, 0);
> +
> +	return 0;
> +}
> +
> +static int ti_pipe3_power_on(struct phy *x)
> +{
> +	struct ti_pipe3 *phy = phy_get_drvdata(x);
> +	int val;
> +	int timeout = PLL_IDLE_TIME;
> +
> +	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
> +	val &= ~PLL_IDLE;
> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
> +
> +	do {
> +		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
> +		if (!(val & PLL_TICOPWDN))
> +			break;
> +		usleep_range(1, 5);
> +	} while (--timeout);
>  
>  	return 0;
>  }
>  
> -static void omap_usb_dpll_relock(struct omap_usb *phy)
> +static void ti_pipe3_dpll_relock(struct ti_pipe3 *phy)
>  {
>  	u32		val;
>  	unsigned long	timeout;
>  
> -	omap_usb_writel(phy->pll_ctrl_base, PLL_GO, SET_PLL_GO);
> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_GO, SET_PLL_GO);
>  
>  	timeout = jiffies + msecs_to_jiffies(20);
>  	do {
> -		val = omap_usb_readl(phy->pll_ctrl_base, PLL_STATUS);
> +		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
>  		if (val & PLL_LOCK)
>  			break;
>  	} while (!WARN_ON(time_after(jiffies, timeout)));
>  }
>  
> -static int omap_usb_dpll_lock(struct omap_usb *phy)
> +static int ti_pipe3_dpll_lock(struct ti_pipe3 *phy)
>  {
>  	u32			val;
>  	unsigned long		rate;
> -	struct usb_dpll_params *dpll_params;
> +	struct pipe3_dpll_params *dpll_params;
>  
>  	rate = clk_get_rate(phy->sys_clk);
> -	dpll_params = omap_usb3_get_dpll_params(rate);
> +	dpll_params = ti_pipe3_get_dpll_params(rate);
>  	if (!dpll_params) {
>  		dev_err(phy->dev,
>  			  "No DPLL configuration for %lu Hz SYS CLK\n", rate);
>  		return -EINVAL;
>  	}
>  
> -	val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
> +	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
>  	val &= ~PLL_REGN_MASK;
>  	val |= dpll_params->n << PLL_REGN_SHIFT;
> -	omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
>  
> -	val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
> +	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
>  	val &= ~PLL_SELFREQDCO_MASK;
>  	val |= dpll_params->freq << PLL_SELFREQDCO_SHIFT;
> -	omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
>  
> -	val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
> +	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
>  	val &= ~PLL_REGM_MASK;
>  	val |= dpll_params->m << PLL_REGM_SHIFT;
> -	omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
>  
> -	val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION4);
> +	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION4);
>  	val &= ~PLL_REGM_F_MASK;
>  	val |= dpll_params->mf << PLL_REGM_F_SHIFT;
> -	omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION4, val);
> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION4, val);
>  
> -	val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION3);
> +	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION3);
>  	val &= ~PLL_SD_MASK;
>  	val |= dpll_params->sd << PLL_SD_SHIFT;
> -	omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION3, val);
> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION3, val);
>  
> -	omap_usb_dpll_relock(phy);
> +	ti_pipe3_dpll_relock(phy);
>  
>  	return 0;
>  }
>  
> -static int omap_usb3_init(struct usb_phy *x)
> +static int ti_pipe3_init(struct phy *x)
>  {
> -	struct omap_usb	*phy = phy_to_omapusb(x);
> +	struct ti_pipe3 *phy = phy_get_drvdata(x);
>  	int ret;
>  
> -	ret = omap_usb_dpll_lock(phy);
> +	ret = ti_pipe3_dpll_lock(phy);
>  	if (ret)
>  		return ret;
>  
> @@ -195,9 +199,18 @@ static int omap_usb3_init(struct usb_phy *x)
>  	return 0;
>  }
>  
> -static int omap_usb3_probe(struct platform_device *pdev)
> +static struct phy_ops ops = {
> +	.init		= ti_pipe3_init,
> +	.power_on	= ti_pipe3_power_on,
> +	.power_off	= ti_pipe3_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static int ti_pipe3_probe(struct platform_device *pdev)
>  {
> -	struct omap_usb *phy;
> +	struct ti_pipe3 *phy;
> +	struct phy *generic_phy;
> +	struct phy_provider *phy_provider;
>  	struct resource *res;
>  	struct device_node *node = pdev->dev.of_node;
>  	struct device_node *control_node;
> @@ -208,7 +221,7 @@ static int omap_usb3_probe(struct platform_device *pdev)
>  
>  	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
>  	if (!phy) {
> -		dev_err(&pdev->dev, "unable to alloc mem for OMAP USB3 PHY\n");
> +		dev_err(&pdev->dev, "unable to alloc mem for TI PIPE3 PHY\n");
>  		return -ENOMEM;
>  	}
>  
> @@ -219,13 +232,6 @@ static int omap_usb3_probe(struct platform_device *pdev)
>  
>  	phy->dev		= &pdev->dev;
>  
> -	phy->phy.dev		= phy->dev;
> -	phy->phy.label		= "omap-usb3";
> -	phy->phy.init		= omap_usb3_init;
> -	phy->phy.set_suspend	= omap_usb3_suspend;
> -	phy->phy.type		= USB_PHY_TYPE_USB3;
> -
> -	phy->is_suspended	= 1;
>  	phy->wkupclk = devm_clk_get(phy->dev, "usb_phy_cm_clk32k");
>  	if (IS_ERR(phy->wkupclk)) {
>  		dev_err(&pdev->dev, "unable to get usb_phy_cm_clk32k\n");
> @@ -251,6 +257,11 @@ static int omap_usb3_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "Failed to get control device phandle\n");
>  		return -EINVAL;
>  	}
> +	phy_provider = devm_of_phy_provider_register(phy->dev,
> +			of_phy_simple_xlate);
> +	if (IS_ERR(phy_provider))
> +		return PTR_ERR(phy_provider);
> +
>  	control_pdev = of_find_device_by_node(control_node);
>  	if (!control_pdev) {
>  		dev_err(&pdev->dev, "Failed to get control device\n");
> @@ -260,23 +271,27 @@ static int omap_usb3_probe(struct platform_device *pdev)
>  	phy->control_dev = &control_pdev->dev;
>  
>  	omap_control_usb_phy_power(phy->control_dev, 0);
> -	usb_add_phy_dev(&phy->phy);
>  
>  	platform_set_drvdata(pdev, phy);
> -
>  	pm_runtime_enable(phy->dev);
> +
> +	generic_phy = devm_phy_create(phy->dev, &ops, NULL);
> +	if (IS_ERR(generic_phy))
> +		return PTR_ERR(generic_phy);
> +
> +	phy_set_drvdata(generic_phy, phy);
> +
>  	pm_runtime_get(&pdev->dev);
>  
>  	return 0;
>  }
>  
> -static int omap_usb3_remove(struct platform_device *pdev)
> +static int ti_pipe3_remove(struct platform_device *pdev)
>  {
> -	struct omap_usb *phy = platform_get_drvdata(pdev);
> +	struct ti_pipe3 *phy = platform_get_drvdata(pdev);
>  
>  	clk_unprepare(phy->wkupclk);
>  	clk_unprepare(phy->optclk);
> -	usb_remove_phy(&phy->phy);
>  	if (!pm_runtime_suspended(&pdev->dev))
>  		pm_runtime_put(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> @@ -286,10 +301,9 @@ static int omap_usb3_remove(struct platform_device *pdev)
>  
>  #ifdef CONFIG_PM_RUNTIME
>  
> -static int omap_usb3_runtime_suspend(struct device *dev)
> +static int ti_pipe3_runtime_suspend(struct device *dev)
>  {
> -	struct platform_device	*pdev = to_platform_device(dev);
> -	struct omap_usb	*phy = platform_get_drvdata(pdev);
> +	struct ti_pipe3	*phy = dev_get_drvdata(dev);
>  
>  	clk_disable(phy->wkupclk);
>  	clk_disable(phy->optclk);
> @@ -297,11 +311,10 @@ static int omap_usb3_runtime_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static int omap_usb3_runtime_resume(struct device *dev)
> +static int ti_pipe3_runtime_resume(struct device *dev)
>  {
>  	u32 ret = 0;
> -	struct platform_device	*pdev = to_platform_device(dev);
> -	struct omap_usb	*phy = platform_get_drvdata(pdev);
> +	struct ti_pipe3	*phy = dev_get_drvdata(dev);
>  
>  	ret = clk_enable(phy->optclk);
>  	if (ret) {
> @@ -324,38 +337,41 @@ err1:
>  	return ret;
>  }
>  
> -static const struct dev_pm_ops omap_usb3_pm_ops = {
> -	SET_RUNTIME_PM_OPS(omap_usb3_runtime_suspend, omap_usb3_runtime_resume,
> -		NULL)
> +static const struct dev_pm_ops ti_pipe3_pm_ops = {
> +	SET_RUNTIME_PM_OPS(ti_pipe3_runtime_suspend,
> +		ti_pipe3_runtime_resume, NULL)
>  };
>  
> -#define DEV_PM_OPS     (&omap_usb3_pm_ops)
> +#define DEV_PM_OPS     (&ti_pipe3_pm_ops)
>  #else
>  #define DEV_PM_OPS     NULL
>  #endif
>  
>  #ifdef CONFIG_OF
> -static const struct of_device_id omap_usb3_id_table[] = {
> +static const struct of_device_id ti_pipe3_id_table[] = {
> +	{ .compatible = "ti,phy-sata" },
> +	{ .compatible = "ti,phy-pcie" },

sata and pcie are not yet supported by this driver so better avoid adding
the compatible strings now.

> +	{ .compatible = "ti,phy-usb3" },
>  	{ .compatible = "ti,omap-usb3" },
>  	{}
>  };
> -MODULE_DEVICE_TABLE(of, omap_usb3_id_table);
> +MODULE_DEVICE_TABLE(of, ti_pipe3_id_table);
>  #endif
>  
> -static struct platform_driver omap_usb3_driver = {
> -	.probe		= omap_usb3_probe,
> -	.remove		= omap_usb3_remove,
> +static struct platform_driver ti_pipe3_driver = {
> +	.probe		= ti_pipe3_probe,
> +	.remove		= ti_pipe3_remove,
>  	.driver		= {
> -		.name	= "omap-usb3",
> +		.name	= "ti-pipe3",
>  		.owner	= THIS_MODULE,
>  		.pm	= DEV_PM_OPS,
> -		.of_match_table = of_match_ptr(omap_usb3_id_table),
> +		.of_match_table = of_match_ptr(ti_pipe3_id_table),
>  	},
>  };
>  
> -module_platform_driver(omap_usb3_driver);
> +module_platform_driver(ti_pipe3_driver);
>  
> -MODULE_ALIAS("platform: omap_usb3");
> +MODULE_ALIAS("platform: ti_pipe3");
>  MODULE_AUTHOR("Texas Instruments Inc.");
> -MODULE_DESCRIPTION("OMAP USB3 phy driver");
> +MODULE_DESCRIPTION("TI PIPE3 phy driver");
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> index 64b8bef..1d4916a 100644
> --- a/drivers/usb/phy/Kconfig
> +++ b/drivers/usb/phy/Kconfig
> @@ -66,17 +66,6 @@ config OMAP_CONTROL_USB
>  	  power on the USB2 PHY is present in OMAP4 and OMAP5. OMAP5 has an
>  	  additional register to power on USB3 PHY.
>  
> -config OMAP_USB3
> -	tristate "OMAP USB3 PHY Driver"
> -	depends on ARCH_OMAP2PLUS || COMPILE_TEST
> -	select OMAP_CONTROL_USB
> -	select USB_PHY
> -	help
> -	  Enable this to support the USB3 PHY that is part of SOC. This
> -	  driver takes care of all the PHY functionality apart from comparator.
> -	  This driver interacts with the "OMAP Control USB Driver" to power
> -	  on/off the PHY.
> -
>  config AM335X_CONTROL_USB
>  	tristate
>  
> diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
> index 9c37361..fa80661 100644
> --- a/drivers/usb/phy/Makefile
> +++ b/drivers/usb/phy/Makefile
> @@ -15,7 +15,6 @@ obj-$(CONFIG_NOP_USB_XCEIV)		+= phy-generic.o
>  obj-$(CONFIG_OMAP_CONTROL_USB)		+= phy-omap-control.o
>  obj-$(CONFIG_AM335X_CONTROL_USB)	+= phy-am335x-control.o
>  obj-$(CONFIG_AM335X_PHY_USB)		+= phy-am335x.o
> -obj-$(CONFIG_OMAP_USB3)			+= phy-omap-usb3.o
>  obj-$(CONFIG_SAMSUNG_USBPHY)		+= phy-samsung-usb.o
>  obj-$(CONFIG_SAMSUNG_USB2PHY)		+= phy-samsung-usb2.o
>  obj-$(CONFIG_SAMSUNG_USB3PHY)		+= phy-samsung-usb3.o
> diff --git a/include/linux/phy/ti_pipe3.h b/include/linux/phy/ti_pipe3.h
> new file mode 100644
> index 0000000..4d42b04
> --- /dev/null
> +++ b/include/linux/phy/ti_pipe3.h
> @@ -0,0 +1,52 @@
> +/*
> + * ti_pipe3.h -- ti pipe3 phy header file

why do you need this file to sit in include/linux/phy/ ?

I don't think there are any external users so better to just merge
it with drivers/phy/phy-ti-pipe3.c

> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __DRIVERS_TI_PIPE3_H
> +#define __DRIVERS_TI_PIPE3_H
> +
> +#include <linux/io.h>
> +
> +struct pipe3_dpll_params {
> +	u16	m;
> +	u8	n;
> +	u8	freq:3;
> +	u8	sd;
> +	u32	mf;
> +};
> +
> +struct ti_pipe3 {
> +	void __iomem		*pll_ctrl_base;
> +	struct device		*dev;
> +	struct device		*control_dev;
> +	struct clk		*wkupclk;
> +	struct clk		*sys_clk;
> +	struct clk		*optclk;
> +};
> +
> +static inline u32 ti_pipe3_readl(void __iomem *addr, unsigned offset)
> +{
> +	return __raw_readl(addr + offset);
> +}
> +
> +static inline void ti_pipe3_writel(void __iomem *addr, unsigned offset,
> +	u32 data)
> +{
> +	__raw_writel(data, addr + offset);
> +}
> +
> +#endif /* __DRIVERS_TI_PIPE3_H */
> 

cheers,
-roger

Re: [PATCH v2 2/7] usb: dwc3: adapt dwc3 core to use Generic PHY Framework

[Kishon Vijay Abraham I]

Hi,

On Wednesday 16 October 2013 06:48 PM, Roger Quadros wrote:
> Hi,
> 
> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote:
>> Adapted dwc3 core to use the Generic PHY Framework. So for init, exit,
>> power_on and power_off the following APIs are used phy_init(), phy_exit(),
>> phy_power_on() and phy_power_off().
>>
>> However using the old USB phy library wont be removed till the PHYs of all
>> other SoC's using dwc3 core is adapted to the Generic PHY Framework.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  Documentation/devicetree/bindings/usb/dwc3.txt |    6 ++-
>>  drivers/usb/dwc3/Kconfig                       |    1 +
>>  drivers/usb/dwc3/core.c                        |   52 ++++++++++++++++++++++++
>>  drivers/usb/dwc3/core.h                        |    7 ++++
>>  drivers/usb/dwc3/platform_data.h               |    2 +
>>  5 files changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index e807635..471366d 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -6,11 +6,13 @@ Required properties:
>>   - compatible: must be "snps,dwc3"
>>   - reg : Address and length of the register set for the device
>>   - interrupts: Interrupts used by the dwc3 controller.
>> +
>> +Optional properties:
>>   - usb-phy : array of phandle for the PHY device.  The first element
>>     in the array is expected to be a handle to the USB2/HS PHY and
>>     the second element is expected to be a handle to the USB3/SS PHY
>> -
>> -Optional properties:
>> + - phys: from the *Generic PHY* bindings
>> + - phy-names: from the *Generic PHY* bindings
>>   - tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>>  
>>  This is usually a subnode to DWC3 glue to which it is connected.
>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>> index 8e385b4..64eed6f 100644
>> --- a/drivers/usb/dwc3/Kconfig
>> +++ b/drivers/usb/dwc3/Kconfig
>> @@ -2,6 +2,7 @@ config USB_DWC3
>>  	tristate "DesignWare USB3 DRD Core Support"
>>  	depends on (USB || USB_GADGET) && HAS_DMA
>>  	select USB_PHY
>> +	select GENERIC_PHY
>>  	select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
>>  	help
>>  	  Say Y or M here if your system has a Dual Role SuperSpeed
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index cb91d70..28bfa5b 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -82,6 +82,12 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
>>  
>>  	usb_phy_init(dwc->usb2_phy);
>>  	usb_phy_init(dwc->usb3_phy);
>> +
>> +	if (dwc->usb2_generic_phy)
>> +		phy_init(dwc->usb2_generic_phy);
>> +	if (dwc->usb3_generic_phy)
>> +		phy_init(dwc->usb3_generic_phy);
>> +
> 
> dwc3_core_soft_reset() is called from dwc3_core_init() which is called from
> dwc3_probe() but before the PHYs are initialized.
> 
> This will cause phy power_on() to be called before phy_init() which is wrong.
> 
>>  	mdelay(100);
>>  
>>  	/* Clear USB3 PHY reset */
>> @@ -343,6 +349,11 @@ static void dwc3_core_exit(struct dwc3 *dwc)
>>  {
>>  	usb_phy_shutdown(dwc->usb2_phy);
>>  	usb_phy_shutdown(dwc->usb3_phy);
>> +
>> +	if (dwc->usb2_generic_phy)
>> +		phy_power_off(dwc->usb2_generic_phy);
>> +	if (dwc->usb3_generic_phy)
>> +		phy_power_off(dwc->usb3_generic_phy);
>>  }
>>  
>>  #define DWC3_ALIGN_MASK		(16 - 1)
>> @@ -439,6 +450,26 @@ static int dwc3_probe(struct platform_device *pdev)
>>  		dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
>>  	}
>>  
>> +	count = of_property_match_string(node, "phy-names", "usb2-phy");
>> +	if (count >= 0 || (pdata && pdata->usb2_generic_phy)) {
>> +		dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
>> +		if (IS_ERR(dwc->usb2_generic_phy)) {
>> +			dev_err(dev, "no usb2 phy configured yet");
>> +			return PTR_ERR(dwc->usb2_generic_phy);
>> +		}
>> +		dwc->usb2_phy = NULL;
>> +	}
>> +
>> +	count = of_property_match_string(node, "phy-names", "usb3-phy");
>> +	if (count >= 0 || (pdata && pdata->usb3_generic_phy)) {
>> +		dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>> +		if (IS_ERR(dwc->usb3_generic_phy)) {
>> +			dev_err(dev, "no usb3 phy configured yet");
>> +			return PTR_ERR(dwc->usb3_generic_phy);
>> +		}
>> +		dwc->usb3_phy = NULL;
>> +	}
>> +
> 
> There are a couple of issues here.
> - The above code must be executed only if it node is valid.

of_property_match_string will give a error value if the node is not present.
*count >= 0* can take care of this.
> - We must either get both PHYs via old method or via new method and not support mix matching
> them. e.g. it is possible with this code that usb2_phy is set and usb3_generic_phy is set.

Why? As long as both the frameworks co-exist (and we support both in dwc3), I
don't see any reason why we shouldn't allow it. We can avoid adding a few more
checks by leaving it the way it is currently.

Thanks
Kishon

Re: [PATCH v2 2/7] usb: dwc3: adapt dwc3 core to use Generic PHY Framework

[Roger Quadros]

On 10/16/2013 04:52 PM, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Wednesday 16 October 2013 06:48 PM, Roger Quadros wrote:
>> Hi,
>>
>> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote:
>>> Adapted dwc3 core to use the Generic PHY Framework. So for init, exit,
>>> power_on and power_off the following APIs are used phy_init(), phy_exit(),
>>> phy_power_on() and phy_power_off().
>>>
>>> However using the old USB phy library wont be removed till the PHYs of all
>>> other SoC's using dwc3 core is adapted to the Generic PHY Framework.
>>>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>>  Documentation/devicetree/bindings/usb/dwc3.txt |    6 ++-
>>>  drivers/usb/dwc3/Kconfig                       |    1 +
>>>  drivers/usb/dwc3/core.c                        |   52 ++++++++++++++++++++++++
>>>  drivers/usb/dwc3/core.h                        |    7 ++++
>>>  drivers/usb/dwc3/platform_data.h               |    2 +
>>>  5 files changed, 66 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> index e807635..471366d 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> @@ -6,11 +6,13 @@ Required properties:
>>>   - compatible: must be "snps,dwc3"
>>>   - reg : Address and length of the register set for the device
>>>   - interrupts: Interrupts used by the dwc3 controller.
>>> +
>>> +Optional properties:
>>>   - usb-phy : array of phandle for the PHY device.  The first element
>>>     in the array is expected to be a handle to the USB2/HS PHY and
>>>     the second element is expected to be a handle to the USB3/SS PHY
>>> -
>>> -Optional properties:
>>> + - phys: from the *Generic PHY* bindings
>>> + - phy-names: from the *Generic PHY* bindings
>>>   - tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>>>  
>>>  This is usually a subnode to DWC3 glue to which it is connected.
>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>> index 8e385b4..64eed6f 100644
>>> --- a/drivers/usb/dwc3/Kconfig
>>> +++ b/drivers/usb/dwc3/Kconfig
>>> @@ -2,6 +2,7 @@ config USB_DWC3
>>>  	tristate "DesignWare USB3 DRD Core Support"
>>>  	depends on (USB || USB_GADGET) && HAS_DMA
>>>  	select USB_PHY
>>> +	select GENERIC_PHY
>>>  	select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
>>>  	help
>>>  	  Say Y or M here if your system has a Dual Role SuperSpeed
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index cb91d70..28bfa5b 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -82,6 +82,12 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
>>>  
>>>  	usb_phy_init(dwc->usb2_phy);
>>>  	usb_phy_init(dwc->usb3_phy);
>>> +
>>> +	if (dwc->usb2_generic_phy)
>>> +		phy_init(dwc->usb2_generic_phy);
>>> +	if (dwc->usb3_generic_phy)
>>> +		phy_init(dwc->usb3_generic_phy);
>>> +
>>
>> dwc3_core_soft_reset() is called from dwc3_core_init() which is called from
>> dwc3_probe() but before the PHYs are initialized.
>>
>> This will cause phy power_on() to be called before phy_init() which is wrong.
>>

You overlooked the above?

>>>  	mdelay(100);
>>>  
>>>  	/* Clear USB3 PHY reset */
>>> @@ -343,6 +349,11 @@ static void dwc3_core_exit(struct dwc3 *dwc)
>>>  {
>>>  	usb_phy_shutdown(dwc->usb2_phy);
>>>  	usb_phy_shutdown(dwc->usb3_phy);
>>> +
>>> +	if (dwc->usb2_generic_phy)
>>> +		phy_power_off(dwc->usb2_generic_phy);
>>> +	if (dwc->usb3_generic_phy)
>>> +		phy_power_off(dwc->usb3_generic_phy);
>>>  }
>>>  
>>>  #define DWC3_ALIGN_MASK		(16 - 1)
>>> @@ -439,6 +450,26 @@ static int dwc3_probe(struct platform_device *pdev)
>>>  		dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
>>>  	}
>>>  
>>> +	count = of_property_match_string(node, "phy-names", "usb2-phy");
>>> +	if (count >= 0 || (pdata && pdata->usb2_generic_phy)) {
>>> +		dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
>>> +		if (IS_ERR(dwc->usb2_generic_phy)) {
>>> +			dev_err(dev, "no usb2 phy configured yet");
>>> +			return PTR_ERR(dwc->usb2_generic_phy);
>>> +		}
>>> +		dwc->usb2_phy = NULL;
>>> +	}
>>> +
>>> +	count = of_property_match_string(node, "phy-names", "usb3-phy");
>>> +	if (count >= 0 || (pdata && pdata->usb3_generic_phy)) {
>>> +		dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>>> +		if (IS_ERR(dwc->usb3_generic_phy)) {
>>> +			dev_err(dev, "no usb3 phy configured yet");
>>> +			return PTR_ERR(dwc->usb3_generic_phy);
>>> +		}
>>> +		dwc->usb3_phy = NULL;
>>> +	}
>>> +
>>
>> There are a couple of issues here.
>> - The above code must be executed only if it node is valid.
> 
> of_property_match_string will give a error value if the node is not present.
> *count >= 0* can take care of this.

OK. 

>> - We must either get both PHYs via old method or via new method and not support mix matching
>> them. e.g. it is possible with this code that usb2_phy is set and usb3_generic_phy is set.
> 
> Why? As long as both the frameworks co-exist (and we support both in dwc3), I
> don't see any reason why we shouldn't allow it. We can avoid adding a few more
> checks by leaving it the way it is currently.

Because it just doesn't seem elegant. Why would you want to even allow both types of PHY
to be used simultaneously?

cheers,
-roger

Re: [PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Felipe Balbi]

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

On Wed, Oct 16, 2013 at 04:27:26PM +0300, Roger Quadros wrote:
> On 10/16/2013 04:10 PM, Kishon Vijay Abraham I wrote:
> > Hi roger,
> > 
> > On Wednesday 16 October 2013 06:33 PM, Roger Quadros wrote:
> >> Hi Kishon,
> >>
> >> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote:
> >>> There can be systems which does not have a external usb_phy, so get
> >>> usb_phy only if dt data indicates the presence of PHY in the case of dt boot or
> >>> if platform_data indicates the presence of PHY. Also remove checking if
> >>> return value is -ENXIO since it's now changed to always enable usb_phy layer.
> >>>
> >>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>> ---
> >>> In usb_get_phy_by_phandle, index 0 always refers to usb2 phy and index 1 always
> >>> refers to usb3 phy. Since we've lived so long with this, this patch will make
> >>> an assumption that if only one entry is populated in *usb-phy* property, it will
> >>> be usb2 phy and the next entry will be usb3 phy.
> >>>
> >>>  drivers/usb/dwc3/Kconfig         |    1 +
> >>>  drivers/usb/dwc3/core.c          |   72 ++++++++++++++++++++------------------
> >>>  drivers/usb/dwc3/platform_data.h |    2 ++
> >>>  3 files changed, 41 insertions(+), 34 deletions(-)
> >>>
> >>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> >>> index 70fc430..8e385b4 100644
> >>> --- a/drivers/usb/dwc3/Kconfig
> >>> +++ b/drivers/usb/dwc3/Kconfig
> >>> @@ -1,6 +1,7 @@
> >>>  config USB_DWC3
> >>>  	tristate "DesignWare USB3 DRD Core Support"
> >>>  	depends on (USB || USB_GADGET) && HAS_DMA
> >>> +	select USB_PHY
> >>>  	select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
> >>>  	help
> >>>  	  Say Y or M here if your system has a Dual Role SuperSpeed
> >>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> >>> index 474162e..cb91d70 100644
> >>> --- a/drivers/usb/dwc3/core.c
> >>> +++ b/drivers/usb/dwc3/core.c
> >>> @@ -354,6 +354,7 @@ static int dwc3_probe(struct platform_device *pdev)
> >>>  	struct device_node	*node = dev->of_node;
> >>>  	struct resource		*res;
> >>>  	struct dwc3		*dwc;
> >>> +	int			count;
> >>>  
> >>>  	int			ret = -ENOMEM;
> >>>  
> >>> @@ -387,16 +388,49 @@ static int dwc3_probe(struct platform_device *pdev)
> >>>  	if (node) {
> >>>  		dwc->maximum_speed = of_usb_get_maximum_speed(node);
> >>>  
> >>> -		dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
> >>> -		dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
> >>> +		count = of_count_phandle_with_args(node, "usb-phy", NULL);
> >>> +		switch (count) {
> >>> +		case 2:
> >>> +			dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev,
> >>> +				"usb-phy", 1);
> >>> +			if (IS_ERR(dwc->usb3_phy)) {
> >>> +				dev_err(dev, "usb3 phy not found\n");
> >>> +				return PTR_ERR(dwc->usb3_phy);
> >>> +			}
> >>> +		case 1:
> >>> +			dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev,
> >>> +				"usb-phy", 0);
> >>> +			if (IS_ERR(dwc->usb2_phy)) {
> >>> +				dev_err(dev, "usb2 phy not found\n");
> >>> +				return PTR_ERR(dwc->usb2_phy);
> >>> +			}
> >>> +			break;
> >>
> >> In the Exynos case, there is only 1 phy and it is the USB3 phy. This code
> >> will wrongly treat it as usb2_phy.
> > 
> > That was the case even before this patch no? Unfortunately the old USB PHY
> > library doesn't have APIs to get PHYs in a better way. If we try modifying the
> > USB PHY library, it'll be kind of duplicating what is already there in the
> > Generic PHY library. I'd rather prefer Exynos guys to use the new framework.
> 
> OK. I agree with you.
> Do you know if there are users of dwc3 other than exynos5250 and omap5?
> If not, we should get rid of the old USB PHY altogether.

Intel's Baytrail, at least. But they don't use DT.

-- 
balbi

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

Re: [PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Felipe Balbi]

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

Hi,

On Wed, Oct 16, 2013 at 01:24:11AM +0530, Kishon Vijay Abraham I wrote:
> There can be systems which does not have a external usb_phy, so get
> usb_phy only if dt data indicates the presence of PHY in the case of dt boot or
> if platform_data indicates the presence of PHY. Also remove checking if
> return value is -ENXIO since it's now changed to always enable usb_phy layer.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

I'm fine with this patch, but one comment below:

> @@ -387,16 +388,49 @@ static int dwc3_probe(struct platform_device *pdev)
>  	if (node) {
>  		dwc->maximum_speed = of_usb_get_maximum_speed(node);
>  
> -		dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
> -		dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
> +		count = of_count_phandle_with_args(node, "usb-phy", NULL);
> +		switch (count) {
> +		case 2:
> +			dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev,
> +				"usb-phy", 1);
> +			if (IS_ERR(dwc->usb3_phy)) {
> +				dev_err(dev, "usb3 phy not found\n");
> +				return PTR_ERR(dwc->usb3_phy);
> +			}
> +		case 1:
> +			dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev,
> +				"usb-phy", 0);
> +			if (IS_ERR(dwc->usb2_phy)) {
> +				dev_err(dev, "usb2 phy not found\n");
> +				return PTR_ERR(dwc->usb2_phy);
> +			}
> +			break;
> +		default:
> +			dev_err(dev, "usb phy nodes not configured\n");
> +		}
>  
>  		dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize");
>  		dwc->dr_mode = of_usb_get_dr_mode(node);
>  	} else if (pdata) {
>  		dwc->maximum_speed = pdata->maximum_speed;
>  
> -		dwc->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> -		dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
> +		if (pdata->usb2_phy) {
> +			dwc->usb2_phy = devm_usb_get_phy(dev,
> +				USB_PHY_TYPE_USB2);
> +			if (IS_ERR(dwc->usb2_phy)) {
> +				dev_err(dev, "usb2 phy not found\n");
> +				return PTR_ERR(dwc->usb2_phy);
> +			}
> +		}
> +
> +		if (pdata->usb3_phy) {
> +			dwc->usb3_phy = devm_usb_get_phy(dev,
> +				USB_PHY_TYPE_USB3);
> +			if (IS_ERR(dwc->usb3_phy)) {
> +				dev_err(dev, "usb3 phy not found\n");
> +				return PTR_ERR(dwc->usb3_phy);
> +			}
> +		}
>  
>  		dwc->needs_fifo_resize = pdata->tx_fifo_resize;
>  		dwc->dr_mode = pdata->dr_mode;
> @@ -409,36 +443,6 @@ static int dwc3_probe(struct platform_device *pdev)
>  	if (dwc->maximum_speed == USB_SPEED_UNKNOWN)
>  		dwc->maximum_speed = USB_SPEED_SUPER;
>  
> -	if (IS_ERR(dwc->usb2_phy)) {
> -		ret = PTR_ERR(dwc->usb2_phy);
> -
> -		/*
> -		 * if -ENXIO is returned, it means PHY layer wasn't
> -		 * enabled, so it makes no sense to return -EPROBE_DEFER
> -		 * in that case, since no PHY driver will ever probe.
> -		 */
> -		if (ret == -ENXIO)
> -			return ret;
> -
> -		dev_err(dev, "no usb2 phy configured\n");
> -		return -EPROBE_DEFER;
> -	}
> -
> -	if (IS_ERR(dwc->usb3_phy)) {
> -		ret = PTR_ERR(dwc->usb3_phy);
> -
> -		/*
> -		 * if -ENXIO is returned, it means PHY layer wasn't
> -		 * enabled, so it makes no sense to return -EPROBE_DEFER
> -		 * in that case, since no PHY driver will ever probe.
> -		 */
> -		if (ret == -ENXIO)
> -			return ret;
> -
> -		dev_err(dev, "no usb3 phy configured\n");
> -		return -EPROBE_DEFER;
> -	}

the idea for doing the error checking here was actually to avoid
duplicating it in all previous cases, as you did. Please keep the error
checking here and you're good to go.

> -
>  	dwc->xhci_resources[0].start = res->start;
>  	dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
>  					DWC3_XHCI_REGS_END;
> diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h
> index 7db34f0..49ffa6d 100644
> --- a/drivers/usb/dwc3/platform_data.h
> +++ b/drivers/usb/dwc3/platform_data.h
> @@ -24,4 +24,6 @@ struct dwc3_platform_data {
>  	enum usb_device_speed maximum_speed;
>  	enum usb_dr_mode dr_mode;
>  	bool tx_fifo_resize;
> +	bool usb2_phy;
> +	bool usb3_phy;

This would look better as a quirks flag, then we could:

unsigned long quirks;
#define DWC3_QUIRK_NO_USB3_PHY	BIT(0)
#define DWC3_QUIRK_NO_USB2_PHY	BIT(1)

...

if (!test_bit(DWC3_QUIRK_NO_USB3_PHY), &dwc->quirks &&
	dwc->has_usb3_phy) {
	grab_usb3_phy();
}

cheers

-- 
balbi

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

Re: [PATCH v2 2/7] usb: dwc3: adapt dwc3 core to use Generic PHY Framework

[Kishon Vijay Abraham I]

Hi,

On Wednesday 16 October 2013 07:53 PM, Roger Quadros wrote:
> On 10/16/2013 04:52 PM, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Wednesday 16 October 2013 06:48 PM, Roger Quadros wrote:
>>> Hi,
>>>
>>> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote:
>>>> Adapted dwc3 core to use the Generic PHY Framework. So for init, exit,
>>>> power_on and power_off the following APIs are used phy_init(), phy_exit(),
>>>> phy_power_on() and phy_power_off().
>>>>
>>>> However using the old USB phy library wont be removed till the PHYs of all
>>>> other SoC's using dwc3 core is adapted to the Generic PHY Framework.
>>>>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/usb/dwc3.txt |    6 ++-
>>>>  drivers/usb/dwc3/Kconfig                       |    1 +
>>>>  drivers/usb/dwc3/core.c                        |   52 ++++++++++++++++++++++++
>>>>  drivers/usb/dwc3/core.h                        |    7 ++++
>>>>  drivers/usb/dwc3/platform_data.h               |    2 +
>>>>  5 files changed, 66 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> index e807635..471366d 100644
>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> @@ -6,11 +6,13 @@ Required properties:
>>>>   - compatible: must be "snps,dwc3"
>>>>   - reg : Address and length of the register set for the device
>>>>   - interrupts: Interrupts used by the dwc3 controller.
>>>> +
>>>> +Optional properties:
>>>>   - usb-phy : array of phandle for the PHY device.  The first element
>>>>     in the array is expected to be a handle to the USB2/HS PHY and
>>>>     the second element is expected to be a handle to the USB3/SS PHY
>>>> -
>>>> -Optional properties:
>>>> + - phys: from the *Generic PHY* bindings
>>>> + - phy-names: from the *Generic PHY* bindings
>>>>   - tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>>>>  
>>>>  This is usually a subnode to DWC3 glue to which it is connected.
>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>>> index 8e385b4..64eed6f 100644
>>>> --- a/drivers/usb/dwc3/Kconfig
>>>> +++ b/drivers/usb/dwc3/Kconfig
>>>> @@ -2,6 +2,7 @@ config USB_DWC3
>>>>  	tristate "DesignWare USB3 DRD Core Support"
>>>>  	depends on (USB || USB_GADGET) && HAS_DMA
>>>>  	select USB_PHY
>>>> +	select GENERIC_PHY
>>>>  	select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
>>>>  	help
>>>>  	  Say Y or M here if your system has a Dual Role SuperSpeed
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index cb91d70..28bfa5b 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -82,6 +82,12 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
>>>>  
>>>>  	usb_phy_init(dwc->usb2_phy);
>>>>  	usb_phy_init(dwc->usb3_phy);
>>>> +
>>>> +	if (dwc->usb2_generic_phy)
>>>> +		phy_init(dwc->usb2_generic_phy);
>>>> +	if (dwc->usb3_generic_phy)
>>>> +		phy_init(dwc->usb3_generic_phy);
>>>> +
>>>
>>> dwc3_core_soft_reset() is called from dwc3_core_init() which is called from
>>> dwc3_probe() but before the PHYs are initialized.
>>>
>>> This will cause phy power_on() to be called before phy_init() which is wrong.
>>>
> 
> You overlooked the above?

No. Forgot to comment back.
Yeah, you are right.. that should be fixed.
> 
>>>>  	mdelay(100);
>>>>  
>>>>  	/* Clear USB3 PHY reset */
>>>> @@ -343,6 +349,11 @@ static void dwc3_core_exit(struct dwc3 *dwc)
>>>>  {
>>>>  	usb_phy_shutdown(dwc->usb2_phy);
>>>>  	usb_phy_shutdown(dwc->usb3_phy);
>>>> +
>>>> +	if (dwc->usb2_generic_phy)
>>>> +		phy_power_off(dwc->usb2_generic_phy);
>>>> +	if (dwc->usb3_generic_phy)
>>>> +		phy_power_off(dwc->usb3_generic_phy);
>>>>  }
>>>>  
>>>>  #define DWC3_ALIGN_MASK		(16 - 1)
>>>> @@ -439,6 +450,26 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>  		dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
>>>>  	}
>>>>  
>>>> +	count = of_property_match_string(node, "phy-names", "usb2-phy");
>>>> +	if (count >= 0 || (pdata && pdata->usb2_generic_phy)) {
>>>> +		dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
>>>> +		if (IS_ERR(dwc->usb2_generic_phy)) {
>>>> +			dev_err(dev, "no usb2 phy configured yet");
>>>> +			return PTR_ERR(dwc->usb2_generic_phy);
>>>> +		}
>>>> +		dwc->usb2_phy = NULL;
>>>> +	}
>>>> +
>>>> +	count = of_property_match_string(node, "phy-names", "usb3-phy");
>>>> +	if (count >= 0 || (pdata && pdata->usb3_generic_phy)) {
>>>> +		dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>>>> +		if (IS_ERR(dwc->usb3_generic_phy)) {
>>>> +			dev_err(dev, "no usb3 phy configured yet");
>>>> +			return PTR_ERR(dwc->usb3_generic_phy);
>>>> +		}
>>>> +		dwc->usb3_phy = NULL;
>>>> +	}
>>>> +
>>>
>>> There are a couple of issues here.
>>> - The above code must be executed only if it node is valid.
>>
>> of_property_match_string will give a error value if the node is not present.
>> *count >= 0* can take care of this.
> 
> OK. 
> 
>>> - We must either get both PHYs via old method or via new method and not support mix matching
>>> them. e.g. it is possible with this code that usb2_phy is set and usb3_generic_phy is set.
>>
>> Why? As long as both the frameworks co-exist (and we support both in dwc3), I
>> don't see any reason why we shouldn't allow it. We can avoid adding a few more
>> checks by leaving it the way it is currently.
> 
> Because it just doesn't seem elegant. Why would you want to even allow both types of PHY
> to be used simultaneously?

We actually don't allow both types of PHYs to be used simultaneously.  We set
usb2_phy/usb3_phy to NULL, if we had got the the generic PHY.
If you just doesn't even want to get PHY by the second method if we have got
PHY by the first method, then we might need to add more *if* checks which IMO
don't make it any elegant either.
Having a clean dt data wont have both types of PHYs anyway.

Thanks
Kishon
> 
> cheers,
> -roger
> 

Re: [PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Kishon Vijay Abraham I]

Hi,

On Thursday 17 October 2013 10:08 PM, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Oct 16, 2013 at 01:24:11AM +0530, Kishon Vijay Abraham I wrote:
>> There can be systems which does not have a external usb_phy, so get
>> usb_phy only if dt data indicates the presence of PHY in the case of dt boot or
>> if platform_data indicates the presence of PHY. Also remove checking if
>> return value is -ENXIO since it's now changed to always enable usb_phy layer.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> 
> I'm fine with this patch, but one comment below:
> 
>> @@ -387,16 +388,49 @@ static int dwc3_probe(struct platform_device *pdev)
>>  	if (node) {
>>  		dwc->maximum_speed = of_usb_get_maximum_speed(node);
>>  
>> -		dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
>> -		dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
>> +		count = of_count_phandle_with_args(node, "usb-phy", NULL);
>> +		switch (count) {
>> +		case 2:
>> +			dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev,
>> +				"usb-phy", 1);
>> +			if (IS_ERR(dwc->usb3_phy)) {
>> +				dev_err(dev, "usb3 phy not found\n");
>> +				return PTR_ERR(dwc->usb3_phy);
>> +			}
>> +		case 1:
>> +			dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev,
>> +				"usb-phy", 0);
>> +			if (IS_ERR(dwc->usb2_phy)) {
>> +				dev_err(dev, "usb2 phy not found\n");
>> +				return PTR_ERR(dwc->usb2_phy);
>> +			}
>> +			break;
>> +		default:
>> +			dev_err(dev, "usb phy nodes not configured\n");
>> +		}
>>  
>>  		dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize");
>>  		dwc->dr_mode = of_usb_get_dr_mode(node);
>>  	} else if (pdata) {
>>  		dwc->maximum_speed = pdata->maximum_speed;
>>  
>> -		dwc->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>> -		dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
>> +		if (pdata->usb2_phy) {
>> +			dwc->usb2_phy = devm_usb_get_phy(dev,
>> +				USB_PHY_TYPE_USB2);
>> +			if (IS_ERR(dwc->usb2_phy)) {
>> +				dev_err(dev, "usb2 phy not found\n");
>> +				return PTR_ERR(dwc->usb2_phy);
>> +			}
>> +		}
>> +
>> +		if (pdata->usb3_phy) {
>> +			dwc->usb3_phy = devm_usb_get_phy(dev,
>> +				USB_PHY_TYPE_USB3);
>> +			if (IS_ERR(dwc->usb3_phy)) {
>> +				dev_err(dev, "usb3 phy not found\n");
>> +				return PTR_ERR(dwc->usb3_phy);
>> +			}
>> +		}
>>  
>>  		dwc->needs_fifo_resize = pdata->tx_fifo_resize;
>>  		dwc->dr_mode = pdata->dr_mode;
>> @@ -409,36 +443,6 @@ static int dwc3_probe(struct platform_device *pdev)
>>  	if (dwc->maximum_speed == USB_SPEED_UNKNOWN)
>>  		dwc->maximum_speed = USB_SPEED_SUPER;
>>  
>> -	if (IS_ERR(dwc->usb2_phy)) {
>> -		ret = PTR_ERR(dwc->usb2_phy);
>> -
>> -		/*
>> -		 * if -ENXIO is returned, it means PHY layer wasn't
>> -		 * enabled, so it makes no sense to return -EPROBE_DEFER
>> -		 * in that case, since no PHY driver will ever probe.
>> -		 */
>> -		if (ret == -ENXIO)
>> -			return ret;
>> -
>> -		dev_err(dev, "no usb2 phy configured\n");
>> -		return -EPROBE_DEFER;
>> -	}
>> -
>> -	if (IS_ERR(dwc->usb3_phy)) {
>> -		ret = PTR_ERR(dwc->usb3_phy);
>> -
>> -		/*
>> -		 * if -ENXIO is returned, it means PHY layer wasn't
>> -		 * enabled, so it makes no sense to return -EPROBE_DEFER
>> -		 * in that case, since no PHY driver will ever probe.
>> -		 */
>> -		if (ret == -ENXIO)
>> -			return ret;
>> -
>> -		dev_err(dev, "no usb3 phy configured\n");
>> -		return -EPROBE_DEFER;
>> -	}
> 
> the idea for doing the error checking here was actually to avoid
> duplicating it in all previous cases, as you did. Please keep the error
> checking here and you're good to go.

Ok.
> 
>> -
>>  	dwc->xhci_resources[0].start = res->start;
>>  	dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
>>  					DWC3_XHCI_REGS_END;
>> diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h
>> index 7db34f0..49ffa6d 100644
>> --- a/drivers/usb/dwc3/platform_data.h
>> +++ b/drivers/usb/dwc3/platform_data.h
>> @@ -24,4 +24,6 @@ struct dwc3_platform_data {
>>  	enum usb_device_speed maximum_speed;
>>  	enum usb_dr_mode dr_mode;
>>  	bool tx_fifo_resize;
>> +	bool usb2_phy;
>> +	bool usb3_phy;
> 
> This would look better as a quirks flag, then we could:
> 
> unsigned long quirks;
> #define DWC3_QUIRK_NO_USB3_PHY	BIT(0)
> #define DWC3_QUIRK_NO_USB2_PHY	BIT(1)
> 
> ...
> 
> if (!test_bit(DWC3_QUIRK_NO_USB3_PHY), &dwc->quirks &&
> 	dwc->has_usb3_phy) {
> 	grab_usb3_phy();
> }

Ok.

Thanks
Kishon

Re: [PATCH v2 2/7] usb: dwc3: adapt dwc3 core to use Generic PHY Framework

[Roger Quadros]

On 10/21/2013 02:55 PM, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Wednesday 16 October 2013 07:53 PM, Roger Quadros wrote:
>> On 10/16/2013 04:52 PM, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Wednesday 16 October 2013 06:48 PM, Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote:
>>>>> Adapted dwc3 core to use the Generic PHY Framework. So for init, exit,
>>>>> power_on and power_off the following APIs are used phy_init(), phy_exit(),
>>>>> phy_power_on() and phy_power_off().
>>>>>
>>>>> However using the old USB phy library wont be removed till the PHYs of all
>>>>> other SoC's using dwc3 core is adapted to the Generic PHY Framework.
>>>>>
>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/usb/dwc3.txt |    6 ++-
>>>>>  drivers/usb/dwc3/Kconfig                       |    1 +
>>>>>  drivers/usb/dwc3/core.c                        |   52 ++++++++++++++++++++++++
>>>>>  drivers/usb/dwc3/core.h                        |    7 ++++
>>>>>  drivers/usb/dwc3/platform_data.h               |    2 +
>>>>>  5 files changed, 66 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> index e807635..471366d 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> @@ -6,11 +6,13 @@ Required properties:
>>>>>   - compatible: must be "snps,dwc3"
>>>>>   - reg : Address and length of the register set for the device
>>>>>   - interrupts: Interrupts used by the dwc3 controller.
>>>>> +
>>>>> +Optional properties:
>>>>>   - usb-phy : array of phandle for the PHY device.  The first element
>>>>>     in the array is expected to be a handle to the USB2/HS PHY and
>>>>>     the second element is expected to be a handle to the USB3/SS PHY
>>>>> -
>>>>> -Optional properties:
>>>>> + - phys: from the *Generic PHY* bindings
>>>>> + - phy-names: from the *Generic PHY* bindings
>>>>>   - tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>>>>>  
>>>>>  This is usually a subnode to DWC3 glue to which it is connected.
>>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>>>> index 8e385b4..64eed6f 100644
>>>>> --- a/drivers/usb/dwc3/Kconfig
>>>>> +++ b/drivers/usb/dwc3/Kconfig
>>>>> @@ -2,6 +2,7 @@ config USB_DWC3
>>>>>  	tristate "DesignWare USB3 DRD Core Support"
>>>>>  	depends on (USB || USB_GADGET) && HAS_DMA
>>>>>  	select USB_PHY
>>>>> +	select GENERIC_PHY
>>>>>  	select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
>>>>>  	help
>>>>>  	  Say Y or M here if your system has a Dual Role SuperSpeed
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index cb91d70..28bfa5b 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -82,6 +82,12 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
>>>>>  
>>>>>  	usb_phy_init(dwc->usb2_phy);
>>>>>  	usb_phy_init(dwc->usb3_phy);
>>>>> +
>>>>> +	if (dwc->usb2_generic_phy)
>>>>> +		phy_init(dwc->usb2_generic_phy);
>>>>> +	if (dwc->usb3_generic_phy)
>>>>> +		phy_init(dwc->usb3_generic_phy);
>>>>> +
>>>>
>>>> dwc3_core_soft_reset() is called from dwc3_core_init() which is called from
>>>> dwc3_probe() but before the PHYs are initialized.
>>>>
>>>> This will cause phy power_on() to be called before phy_init() which is wrong.
>>>>
>>
>> You overlooked the above?
> 
> No. Forgot to comment back.
> Yeah, you are right.. that should be fixed.
>>
>>>>>  	mdelay(100);
>>>>>  
>>>>>  	/* Clear USB3 PHY reset */
>>>>> @@ -343,6 +349,11 @@ static void dwc3_core_exit(struct dwc3 *dwc)
>>>>>  {
>>>>>  	usb_phy_shutdown(dwc->usb2_phy);
>>>>>  	usb_phy_shutdown(dwc->usb3_phy);
>>>>> +
>>>>> +	if (dwc->usb2_generic_phy)
>>>>> +		phy_power_off(dwc->usb2_generic_phy);
>>>>> +	if (dwc->usb3_generic_phy)
>>>>> +		phy_power_off(dwc->usb3_generic_phy);
>>>>>  }
>>>>>  
>>>>>  #define DWC3_ALIGN_MASK		(16 - 1)
>>>>> @@ -439,6 +450,26 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>>  		dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
>>>>>  	}
>>>>>  
>>>>> +	count = of_property_match_string(node, "phy-names", "usb2-phy");
>>>>> +	if (count >= 0 || (pdata && pdata->usb2_generic_phy)) {
>>>>> +		dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
>>>>> +		if (IS_ERR(dwc->usb2_generic_phy)) {
>>>>> +			dev_err(dev, "no usb2 phy configured yet");
>>>>> +			return PTR_ERR(dwc->usb2_generic_phy);
>>>>> +		}
>>>>> +		dwc->usb2_phy = NULL;
>>>>> +	}
>>>>> +
>>>>> +	count = of_property_match_string(node, "phy-names", "usb3-phy");
>>>>> +	if (count >= 0 || (pdata && pdata->usb3_generic_phy)) {
>>>>> +		dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>>>>> +		if (IS_ERR(dwc->usb3_generic_phy)) {
>>>>> +			dev_err(dev, "no usb3 phy configured yet");
>>>>> +			return PTR_ERR(dwc->usb3_generic_phy);
>>>>> +		}
>>>>> +		dwc->usb3_phy = NULL;
>>>>> +	}
>>>>> +
>>>>
>>>> There are a couple of issues here.
>>>> - The above code must be executed only if it node is valid.
>>>
>>> of_property_match_string will give a error value if the node is not present.
>>> *count >= 0* can take care of this.
>>
>> OK. 
>>
>>>> - We must either get both PHYs via old method or via new method and not support mix matching
>>>> them. e.g. it is possible with this code that usb2_phy is set and usb3_generic_phy is set.
>>>
>>> Why? As long as both the frameworks co-exist (and we support both in dwc3), I
>>> don't see any reason why we shouldn't allow it. We can avoid adding a few more
>>> checks by leaving it the way it is currently.
>>
>> Because it just doesn't seem elegant. Why would you want to even allow both types of PHY
>> to be used simultaneously?
> 
> We actually don't allow both types of PHYs to be used simultaneously.  We set
> usb2_phy/usb3_phy to NULL, if we had got the the generic PHY.
> If you just doesn't even want to get PHY by the second method if we have got
> PHY by the first method, then we might need to add more *if* checks which IMO
> don't make it any elegant either.
> Having a clean dt data wont have both types of PHYs anyway.

OK.

cheers,
-roger

Re: [PATCH v2 4/7] drivers: phy: usb3/pipe3: Adapt pipe3 driver to Generic PHY Framework

[Kishon Vijay Abraham I]

Hi,

On Wednesday 16 October 2013 07:10 PM, Roger Quadros wrote:
> Hi,
> 
> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote:
>> Adapted omap-usb3 PHY driver to Generic PHY Framework and moved phy-omap-usb3
>> driver in drivers/usb/phy to drivers/phy and also renamed the file to
>> phy-ti-pipe3 since this same driver will be used for SATA PHY and
>> PCIE PHY.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/phy/Kconfig                                |   10 +
>>  drivers/phy/Makefile                               |    1 +
>>  .../phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c}     |  206 +++++++++++---------
>>  drivers/usb/phy/Kconfig                            |   11 --
>>  drivers/usb/phy/Makefile                           |    1 -
>>  include/linux/phy/ti_pipe3.h                       |   52 +++++
>>  6 files changed, 174 insertions(+), 107 deletions(-)
>>  rename drivers/{usb/phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c} (57%)
>>  create mode 100644 include/linux/phy/ti_pipe3.h
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index ac239ac..1158943 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -27,6 +27,16 @@ config OMAP_USB2
>>  	  The USB OTG controller communicates with the comparator using this
>>  	  driver.
>>  
>> +config TI_PIPE3
>> +	tristate "TI PIPE3 PHY Driver"
>> +	select GENERIC_PHY
>> +	select OMAP_CONTROL_USB
> 
> The original OMAP_USB3 config had this
> 	depends on ARCH_OMAP2PLUS || COMPILE_TEST
> 
> You need the 'depends on ARCH_OMAP2PLUS' else you'll risk build failure on non OMAP
> platforms, because OMAP_CONTROL_USB depends on ARCH_OMAP2PLUS.

huh missed that. Thanks for spotting this.
> 
>> +	help
>> +	  Enable this to support the PIPE3 PHY that is part of TI SOCs. This
>> +	  driver takes care of all the PHY functionality apart from comparator.
>> +	  This driver interacts with the "OMAP Control PHY Driver" to power
>> +	  on/off the PHY.
>> +
>>  config TWL4030_USB
>>  	tristate "TWL4030 USB Transceiver Driver"
>>  	depends on TWL4030_CORE && REGULATOR_TWL4030 && USB_MUSB_OMAP2PLUS
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index 0dd8a98..42ccab4 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -4,4 +4,5 @@
>>  
>>  obj-$(CONFIG_GENERIC_PHY)	+= phy-core.o
>>  obj-$(CONFIG_OMAP_USB2)		+= phy-omap-usb2.o
>> +obj-$(CONFIG_TI_PIPE3)		+= phy-ti-pipe3.o
>>  obj-$(CONFIG_TWL4030_USB)	+= phy-twl4030-usb.o
>> diff --git a/drivers/usb/phy/phy-omap-usb3.c b/drivers/phy/phy-ti-pipe3.c
>> similarity index 57%
>> rename from drivers/usb/phy/phy-omap-usb3.c
>> rename to drivers/phy/phy-ti-pipe3.c
>> index 0c6ba29..31e8397 100644
>> --- a/drivers/usb/phy/phy-omap-usb3.c
>> +++ b/drivers/phy/phy-ti-pipe3.c
>> @@ -1,5 +1,5 @@
>>  /*
>> - * omap-usb3 - USB PHY, talking to dwc3 controller in OMAP.
>> + * phy-ti-pipe3 - PIPE3 PHY driver for SATA, USB and PCIE.
> 
> SATA and PCIe is not yet supported so no need to mention about it now.

hmm Ok.
> 
>>   *
>>   * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
>>   * This program is free software; you can redistribute it and/or modify
>> @@ -19,7 +19,8 @@
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/slab.h>
>> -#include <linux/usb/omap_usb.h>
>> +#include <linux/phy/ti_pipe3.h>
>> +#include <linux/phy/phy.h>
>>  #include <linux/of.h>
>>  #include <linux/clk.h>
>>  #include <linux/err.h>
>> @@ -52,17 +53,17 @@
>>  
>>  /*
>>   * This is an Empirical value that works, need to confirm the actual
>> - * value required for the USB3PHY_PLL_CONFIGURATION2.PLL_IDLE status
>> - * to be correctly reflected in the USB3PHY_PLL_STATUS register.
>> + * value required for the PIPE3PHY_PLL_CONFIGURATION2.PLL_IDLE status
>> + * to be correctly reflected in the PIPE3PHY_PLL_STATUS register.
>>   */
>>  # define PLL_IDLE_TIME  100;
>>  
>> -struct usb_dpll_map {
>> +struct pipe3_dpll_map {
>>  	unsigned long rate;
>> -	struct usb_dpll_params params;
>> +	struct pipe3_dpll_params params;
>>  };
>>  
>> -static struct usb_dpll_map dpll_map[] = {
>> +static struct pipe3_dpll_map dpll_map[] = {
>>  	{12000000, {1250, 5, 4, 20, 0} },	/* 12 MHz */
>>  	{16800000, {3125, 20, 4, 20, 0} },	/* 16.8 MHz */
>>  	{19200000, {1172, 8, 4, 20, 65537} },	/* 19.2 MHz */
>> @@ -71,7 +72,7 @@ static struct usb_dpll_map dpll_map[] = {
>>  	{38400000, {3125, 47, 4, 20, 92843} },	/* 38.4 MHz */
>>  };
>>  
>> -static struct usb_dpll_params *omap_usb3_get_dpll_params(unsigned long rate)
>> +static struct pipe3_dpll_params *ti_pipe3_get_dpll_params(unsigned long rate)
>>  {
>>  	int i;
>>  
>> @@ -83,110 +84,113 @@ static struct usb_dpll_params *omap_usb3_get_dpll_params(unsigned long rate)
>>  	return NULL;
>>  }
>>  
>> -static int omap_usb3_suspend(struct usb_phy *x, int suspend)
>> +static int ti_pipe3_power_off(struct phy *x)
>>  {
>> -	struct omap_usb *phy = phy_to_omapusb(x);
>> -	int	val;
>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);
>> +	int val;
>>  	int timeout = PLL_IDLE_TIME;
>>  
>> -	if (suspend && !phy->is_suspended) {
>> -		val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
>> -		val |= PLL_IDLE;
>> -		omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
>> -
>> -		do {
>> -			val = omap_usb_readl(phy->pll_ctrl_base, PLL_STATUS);
>> -			if (val & PLL_TICOPWDN)
>> -				break;
>> -			udelay(1);
>> -		} while (--timeout);
>> -
>> -		omap_control_usb_phy_power(phy->control_dev, 0);
>> -
>> -		phy->is_suspended	= 1;
>> -	} else if (!suspend && phy->is_suspended) {
>> -		phy->is_suspended	= 0;
>> -
>> -		val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
>> -		val &= ~PLL_IDLE;
>> -		omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
>> -
>> -		do {
>> -			val = omap_usb_readl(phy->pll_ctrl_base, PLL_STATUS);
>> -			if (!(val & PLL_TICOPWDN))
>> -				break;
>> -			udelay(1);
>> -		} while (--timeout);
>> -	}
>> +	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
>> +	val |= PLL_IDLE;
>> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
>> +
>> +	do {
>> +		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
>> +		if (val & PLL_TICOPWDN)
>> +			break;
>> +		usleep_range(1, 5);
>> +	} while (--timeout);
>> +
>> +	omap_control_usb_phy_power(phy->control_dev, 0);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ti_pipe3_power_on(struct phy *x)
>> +{
>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);
>> +	int val;
>> +	int timeout = PLL_IDLE_TIME;
>> +
>> +	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
>> +	val &= ~PLL_IDLE;
>> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
>> +
>> +	do {
>> +		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
>> +		if (!(val & PLL_TICOPWDN))
>> +			break;
>> +		usleep_range(1, 5);
>> +	} while (--timeout);
>>  
>>  	return 0;
>>  }
>>  
>> -static void omap_usb_dpll_relock(struct omap_usb *phy)
>> +static void ti_pipe3_dpll_relock(struct ti_pipe3 *phy)
>>  {
>>  	u32		val;
>>  	unsigned long	timeout;
>>  
>> -	omap_usb_writel(phy->pll_ctrl_base, PLL_GO, SET_PLL_GO);
>> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_GO, SET_PLL_GO);
>>  
>>  	timeout = jiffies + msecs_to_jiffies(20);
>>  	do {
>> -		val = omap_usb_readl(phy->pll_ctrl_base, PLL_STATUS);
>> +		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
>>  		if (val & PLL_LOCK)
>>  			break;
>>  	} while (!WARN_ON(time_after(jiffies, timeout)));
>>  }
>>  
>> -static int omap_usb_dpll_lock(struct omap_usb *phy)
>> +static int ti_pipe3_dpll_lock(struct ti_pipe3 *phy)
>>  {
>>  	u32			val;
>>  	unsigned long		rate;
>> -	struct usb_dpll_params *dpll_params;
>> +	struct pipe3_dpll_params *dpll_params;
>>  
>>  	rate = clk_get_rate(phy->sys_clk);
>> -	dpll_params = omap_usb3_get_dpll_params(rate);
>> +	dpll_params = ti_pipe3_get_dpll_params(rate);
>>  	if (!dpll_params) {
>>  		dev_err(phy->dev,
>>  			  "No DPLL configuration for %lu Hz SYS CLK\n", rate);
>>  		return -EINVAL;
>>  	}
>>  
>> -	val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
>> +	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
>>  	val &= ~PLL_REGN_MASK;
>>  	val |= dpll_params->n << PLL_REGN_SHIFT;
>> -	omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
>> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
>>  
>> -	val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
>> +	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
>>  	val &= ~PLL_SELFREQDCO_MASK;
>>  	val |= dpll_params->freq << PLL_SELFREQDCO_SHIFT;
>> -	omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
>> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
>>  
>> -	val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
>> +	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
>>  	val &= ~PLL_REGM_MASK;
>>  	val |= dpll_params->m << PLL_REGM_SHIFT;
>> -	omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
>> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
>>  
>> -	val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION4);
>> +	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION4);
>>  	val &= ~PLL_REGM_F_MASK;
>>  	val |= dpll_params->mf << PLL_REGM_F_SHIFT;
>> -	omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION4, val);
>> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION4, val);
>>  
>> -	val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION3);
>> +	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION3);
>>  	val &= ~PLL_SD_MASK;
>>  	val |= dpll_params->sd << PLL_SD_SHIFT;
>> -	omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION3, val);
>> +	ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION3, val);
>>  
>> -	omap_usb_dpll_relock(phy);
>> +	ti_pipe3_dpll_relock(phy);
>>  
>>  	return 0;
>>  }
>>  
>> -static int omap_usb3_init(struct usb_phy *x)
>> +static int ti_pipe3_init(struct phy *x)
>>  {
>> -	struct omap_usb	*phy = phy_to_omapusb(x);
>> +	struct ti_pipe3 *phy = phy_get_drvdata(x);
>>  	int ret;
>>  
>> -	ret = omap_usb_dpll_lock(phy);
>> +	ret = ti_pipe3_dpll_lock(phy);
>>  	if (ret)
>>  		return ret;
>>  
>> @@ -195,9 +199,18 @@ static int omap_usb3_init(struct usb_phy *x)
>>  	return 0;
>>  }
>>  
>> -static int omap_usb3_probe(struct platform_device *pdev)
>> +static struct phy_ops ops = {
>> +	.init		= ti_pipe3_init,
>> +	.power_on	= ti_pipe3_power_on,
>> +	.power_off	= ti_pipe3_power_off,
>> +	.owner		= THIS_MODULE,
>> +};
>> +
>> +static int ti_pipe3_probe(struct platform_device *pdev)
>>  {
>> -	struct omap_usb *phy;
>> +	struct ti_pipe3 *phy;
>> +	struct phy *generic_phy;
>> +	struct phy_provider *phy_provider;
>>  	struct resource *res;
>>  	struct device_node *node = pdev->dev.of_node;
>>  	struct device_node *control_node;
>> @@ -208,7 +221,7 @@ static int omap_usb3_probe(struct platform_device *pdev)
>>  
>>  	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
>>  	if (!phy) {
>> -		dev_err(&pdev->dev, "unable to alloc mem for OMAP USB3 PHY\n");
>> +		dev_err(&pdev->dev, "unable to alloc mem for TI PIPE3 PHY\n");
>>  		return -ENOMEM;
>>  	}
>>  
>> @@ -219,13 +232,6 @@ static int omap_usb3_probe(struct platform_device *pdev)
>>  
>>  	phy->dev		= &pdev->dev;
>>  
>> -	phy->phy.dev		= phy->dev;
>> -	phy->phy.label		= "omap-usb3";
>> -	phy->phy.init		= omap_usb3_init;
>> -	phy->phy.set_suspend	= omap_usb3_suspend;
>> -	phy->phy.type		= USB_PHY_TYPE_USB3;
>> -
>> -	phy->is_suspended	= 1;
>>  	phy->wkupclk = devm_clk_get(phy->dev, "usb_phy_cm_clk32k");
>>  	if (IS_ERR(phy->wkupclk)) {
>>  		dev_err(&pdev->dev, "unable to get usb_phy_cm_clk32k\n");
>> @@ -251,6 +257,11 @@ static int omap_usb3_probe(struct platform_device *pdev)
>>  		dev_err(&pdev->dev, "Failed to get control device phandle\n");
>>  		return -EINVAL;
>>  	}
>> +	phy_provider = devm_of_phy_provider_register(phy->dev,
>> +			of_phy_simple_xlate);
>> +	if (IS_ERR(phy_provider))
>> +		return PTR_ERR(phy_provider);
>> +
>>  	control_pdev = of_find_device_by_node(control_node);
>>  	if (!control_pdev) {
>>  		dev_err(&pdev->dev, "Failed to get control device\n");
>> @@ -260,23 +271,27 @@ static int omap_usb3_probe(struct platform_device *pdev)
>>  	phy->control_dev = &control_pdev->dev;
>>  
>>  	omap_control_usb_phy_power(phy->control_dev, 0);
>> -	usb_add_phy_dev(&phy->phy);
>>  
>>  	platform_set_drvdata(pdev, phy);
>> -
>>  	pm_runtime_enable(phy->dev);
>> +
>> +	generic_phy = devm_phy_create(phy->dev, &ops, NULL);
>> +	if (IS_ERR(generic_phy))
>> +		return PTR_ERR(generic_phy);
>> +
>> +	phy_set_drvdata(generic_phy, phy);
>> +
>>  	pm_runtime_get(&pdev->dev);
>>  
>>  	return 0;
>>  }
>>  
>> -static int omap_usb3_remove(struct platform_device *pdev)
>> +static int ti_pipe3_remove(struct platform_device *pdev)
>>  {
>> -	struct omap_usb *phy = platform_get_drvdata(pdev);
>> +	struct ti_pipe3 *phy = platform_get_drvdata(pdev);
>>  
>>  	clk_unprepare(phy->wkupclk);
>>  	clk_unprepare(phy->optclk);
>> -	usb_remove_phy(&phy->phy);
>>  	if (!pm_runtime_suspended(&pdev->dev))
>>  		pm_runtime_put(&pdev->dev);
>>  	pm_runtime_disable(&pdev->dev);
>> @@ -286,10 +301,9 @@ static int omap_usb3_remove(struct platform_device *pdev)
>>  
>>  #ifdef CONFIG_PM_RUNTIME
>>  
>> -static int omap_usb3_runtime_suspend(struct device *dev)
>> +static int ti_pipe3_runtime_suspend(struct device *dev)
>>  {
>> -	struct platform_device	*pdev = to_platform_device(dev);
>> -	struct omap_usb	*phy = platform_get_drvdata(pdev);
>> +	struct ti_pipe3	*phy = dev_get_drvdata(dev);
>>  
>>  	clk_disable(phy->wkupclk);
>>  	clk_disable(phy->optclk);
>> @@ -297,11 +311,10 @@ static int omap_usb3_runtime_suspend(struct device *dev)
>>  	return 0;
>>  }
>>  
>> -static int omap_usb3_runtime_resume(struct device *dev)
>> +static int ti_pipe3_runtime_resume(struct device *dev)
>>  {
>>  	u32 ret = 0;
>> -	struct platform_device	*pdev = to_platform_device(dev);
>> -	struct omap_usb	*phy = platform_get_drvdata(pdev);
>> +	struct ti_pipe3	*phy = dev_get_drvdata(dev);
>>  
>>  	ret = clk_enable(phy->optclk);
>>  	if (ret) {
>> @@ -324,38 +337,41 @@ err1:
>>  	return ret;
>>  }
>>  
>> -static const struct dev_pm_ops omap_usb3_pm_ops = {
>> -	SET_RUNTIME_PM_OPS(omap_usb3_runtime_suspend, omap_usb3_runtime_resume,
>> -		NULL)
>> +static const struct dev_pm_ops ti_pipe3_pm_ops = {
>> +	SET_RUNTIME_PM_OPS(ti_pipe3_runtime_suspend,
>> +		ti_pipe3_runtime_resume, NULL)
>>  };
>>  
>> -#define DEV_PM_OPS     (&omap_usb3_pm_ops)
>> +#define DEV_PM_OPS     (&ti_pipe3_pm_ops)
>>  #else
>>  #define DEV_PM_OPS     NULL
>>  #endif
>>  
>>  #ifdef CONFIG_OF
>> -static const struct of_device_id omap_usb3_id_table[] = {
>> +static const struct of_device_id ti_pipe3_id_table[] = {
>> +	{ .compatible = "ti,phy-sata" },
>> +	{ .compatible = "ti,phy-pcie" },
> 
> sata and pcie are not yet supported by this driver so better avoid adding
> the compatible strings now.

Ok.
> 
>> +	{ .compatible = "ti,phy-usb3" },
>>  	{ .compatible = "ti,omap-usb3" },
>>  	{}
>>  };
>> -MODULE_DEVICE_TABLE(of, omap_usb3_id_table);
>> +MODULE_DEVICE_TABLE(of, ti_pipe3_id_table);
>>  #endif
>>  
>> -static struct platform_driver omap_usb3_driver = {
>> -	.probe		= omap_usb3_probe,
>> -	.remove		= omap_usb3_remove,
>> +static struct platform_driver ti_pipe3_driver = {
>> +	.probe		= ti_pipe3_probe,
>> +	.remove		= ti_pipe3_remove,
>>  	.driver		= {
>> -		.name	= "omap-usb3",
>> +		.name	= "ti-pipe3",
>>  		.owner	= THIS_MODULE,
>>  		.pm	= DEV_PM_OPS,
>> -		.of_match_table = of_match_ptr(omap_usb3_id_table),
>> +		.of_match_table = of_match_ptr(ti_pipe3_id_table),
>>  	},
>>  };
>>  
>> -module_platform_driver(omap_usb3_driver);
>> +module_platform_driver(ti_pipe3_driver);
>>  
>> -MODULE_ALIAS("platform: omap_usb3");
>> +MODULE_ALIAS("platform: ti_pipe3");
>>  MODULE_AUTHOR("Texas Instruments Inc.");
>> -MODULE_DESCRIPTION("OMAP USB3 phy driver");
>> +MODULE_DESCRIPTION("TI PIPE3 phy driver");
>>  MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
>> index 64b8bef..1d4916a 100644
>> --- a/drivers/usb/phy/Kconfig
>> +++ b/drivers/usb/phy/Kconfig
>> @@ -66,17 +66,6 @@ config OMAP_CONTROL_USB
>>  	  power on the USB2 PHY is present in OMAP4 and OMAP5. OMAP5 has an
>>  	  additional register to power on USB3 PHY.
>>  
>> -config OMAP_USB3
>> -	tristate "OMAP USB3 PHY Driver"
>> -	depends on ARCH_OMAP2PLUS || COMPILE_TEST
>> -	select OMAP_CONTROL_USB
>> -	select USB_PHY
>> -	help
>> -	  Enable this to support the USB3 PHY that is part of SOC. This
>> -	  driver takes care of all the PHY functionality apart from comparator.
>> -	  This driver interacts with the "OMAP Control USB Driver" to power
>> -	  on/off the PHY.
>> -
>>  config AM335X_CONTROL_USB
>>  	tristate
>>  
>> diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
>> index 9c37361..fa80661 100644
>> --- a/drivers/usb/phy/Makefile
>> +++ b/drivers/usb/phy/Makefile
>> @@ -15,7 +15,6 @@ obj-$(CONFIG_NOP_USB_XCEIV)		+= phy-generic.o
>>  obj-$(CONFIG_OMAP_CONTROL_USB)		+= phy-omap-control.o
>>  obj-$(CONFIG_AM335X_CONTROL_USB)	+= phy-am335x-control.o
>>  obj-$(CONFIG_AM335X_PHY_USB)		+= phy-am335x.o
>> -obj-$(CONFIG_OMAP_USB3)			+= phy-omap-usb3.o
>>  obj-$(CONFIG_SAMSUNG_USBPHY)		+= phy-samsung-usb.o
>>  obj-$(CONFIG_SAMSUNG_USB2PHY)		+= phy-samsung-usb2.o
>>  obj-$(CONFIG_SAMSUNG_USB3PHY)		+= phy-samsung-usb3.o
>> diff --git a/include/linux/phy/ti_pipe3.h b/include/linux/phy/ti_pipe3.h
>> new file mode 100644
>> index 0000000..4d42b04
>> --- /dev/null
>> +++ b/include/linux/phy/ti_pipe3.h
>> @@ -0,0 +1,52 @@
>> +/*
>> + * ti_pipe3.h -- ti pipe3 phy header file
> 
> why do you need this file to sit in include/linux/phy/ ?
> 
> I don't think there are any external users so better to just merge
> it with drivers/phy/phy-ti-pipe3.c

yeah. Makes sense.

Thanks
Kishon

Re: [PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Vivek Gautam]

Dear Kishon, Roger


On Wed, Oct 16, 2013 at 6:40 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> Hi roger,
>
> On Wednesday 16 October 2013 06:33 PM, Roger Quadros wrote:
>> Hi Kishon,

Apologies for missing this thread for so long.

>>
>> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote:
>>> There can be systems which does not have a external usb_phy, so get
>>> usb_phy only if dt data indicates the presence of PHY in the case of dt boot or
>>> if platform_data indicates the presence of PHY. Also remove checking if
>>> return value is -ENXIO since it's now changed to always enable usb_phy layer.
>>>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>> In usb_get_phy_by_phandle, index 0 always refers to usb2 phy and index 1 always
>>> refers to usb3 phy. Since we've lived so long with this, this patch will make
>>> an assumption that if only one entry is populated in *usb-phy* property, it will
>>> be usb2 phy and the next entry will be usb3 phy.
>>>
>>>  drivers/usb/dwc3/Kconfig         |    1 +
>>>  drivers/usb/dwc3/core.c          |   72 ++++++++++++++++++++------------------
>>>  drivers/usb/dwc3/platform_data.h |    2 ++
>>>  3 files changed, 41 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>> index 70fc430..8e385b4 100644
>>> --- a/drivers/usb/dwc3/Kconfig
>>> +++ b/drivers/usb/dwc3/Kconfig
>>> @@ -1,6 +1,7 @@
>>>  config USB_DWC3
>>>      tristate "DesignWare USB3 DRD Core Support"
>>>      depends on (USB || USB_GADGET) && HAS_DMA
>>> +    select USB_PHY
>>>      select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
>>>      help
>>>        Say Y or M here if your system has a Dual Role SuperSpeed
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 474162e..cb91d70 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -354,6 +354,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>>      struct device_node      *node = dev->of_node;
>>>      struct resource         *res;
>>>      struct dwc3             *dwc;
>>> +    int                     count;
>>>
>>>      int                     ret = -ENOMEM;
>>>
>>> @@ -387,16 +388,49 @@ static int dwc3_probe(struct platform_device *pdev)
>>>      if (node) {
>>>              dwc->maximum_speed = of_usb_get_maximum_speed(node);
>>>
>>> -            dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
>>> -            dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
>>> +            count = of_count_phandle_with_args(node, "usb-phy", NULL);
>>> +            switch (count) {
>>> +            case 2:
>>> +                    dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev,
>>> +                            "usb-phy", 1);
>>> +                    if (IS_ERR(dwc->usb3_phy)) {
>>> +                            dev_err(dev, "usb3 phy not found\n");
>>> +                            return PTR_ERR(dwc->usb3_phy);
>>> +                    }
>>> +            case 1:
>>> +                    dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev,
>>> +                            "usb-phy", 0);
>>> +                    if (IS_ERR(dwc->usb2_phy)) {
>>> +                            dev_err(dev, "usb2 phy not found\n");
>>> +                            return PTR_ERR(dwc->usb2_phy);
>>> +                    }
>>> +                    break;
>>
>> In the Exynos case, there is only 1 phy and it is the USB3 phy. This code
>> will wrongly treat it as usb2_phy.

Thank you Roger for your concern regarding Exynos case.
It's true that, Exynos5 series of SoCs have got only one IP block for
DWC3'c PHY.
This block is actually a combo of USB2 phy (UTMI+) as well as USB3 phy (PIPE3).
And the same is served by a single USB phy driver. This is also
clarified in the thread : https://lkml.org/lkml/2013/11/5/160

>
> That was the case even before this patch no? Unfortunately the old USB PHY
> library doesn't have APIs to get PHYs in a better way. If we try modifying the
> USB PHY library, it'll be kind of duplicating what is already there in the
> Generic PHY library. I'd rather prefer Exynos guys to use the new framework.
>
> Thanks
> Kishon
> --
> 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



-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

Re: [PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Vivek Gautam]

On Tue, Nov 5, 2013 at 11:41 PM, Vivek Gautam <gautamvivek1987@gmail.com> wrote:
> Dear Kishon, Roger
>
>
> On Wed, Oct 16, 2013 at 6:40 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>> Hi roger,
>>
>> On Wednesday 16 October 2013 06:33 PM, Roger Quadros wrote:
>>> Hi Kishon,
>
> Apologies for missing this thread for so long.
>
>>>
>>> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote:
>>>> There can be systems which does not have a external usb_phy, so get
>>>> usb_phy only if dt data indicates the presence of PHY in the case of dt boot or
>>>> if platform_data indicates the presence of PHY. Also remove checking if
>>>> return value is -ENXIO since it's now changed to always enable usb_phy layer.
>>>>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> ---
>>>> In usb_get_phy_by_phandle, index 0 always refers to usb2 phy and index 1 always
>>>> refers to usb3 phy. Since we've lived so long with this, this patch will make
>>>> an assumption that if only one entry is populated in *usb-phy* property, it will
>>>> be usb2 phy and the next entry will be usb3 phy.
>>>>
>>>>  drivers/usb/dwc3/Kconfig         |    1 +
>>>>  drivers/usb/dwc3/core.c          |   72 ++++++++++++++++++++------------------
>>>>  drivers/usb/dwc3/platform_data.h |    2 ++
>>>>  3 files changed, 41 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>>> index 70fc430..8e385b4 100644
>>>> --- a/drivers/usb/dwc3/Kconfig
>>>> +++ b/drivers/usb/dwc3/Kconfig
>>>> @@ -1,6 +1,7 @@
>>>>  config USB_DWC3
>>>>      tristate "DesignWare USB3 DRD Core Support"
>>>>      depends on (USB || USB_GADGET) && HAS_DMA
>>>> +    select USB_PHY
>>>>      select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
>>>>      help
>>>>        Say Y or M here if your system has a Dual Role SuperSpeed
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 474162e..cb91d70 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -354,6 +354,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>      struct device_node      *node = dev->of_node;
>>>>      struct resource         *res;
>>>>      struct dwc3             *dwc;
>>>> +    int                     count;
>>>>
>>>>      int                     ret = -ENOMEM;
>>>>
>>>> @@ -387,16 +388,49 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>      if (node) {
>>>>              dwc->maximum_speed = of_usb_get_maximum_speed(node);
>>>>
>>>> -            dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
>>>> -            dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
>>>> +            count = of_count_phandle_with_args(node, "usb-phy", NULL);
>>>> +            switch (count) {
>>>> +            case 2:
>>>> +                    dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev,
>>>> +                            "usb-phy", 1);
>>>> +                    if (IS_ERR(dwc->usb3_phy)) {
>>>> +                            dev_err(dev, "usb3 phy not found\n");
>>>> +                            return PTR_ERR(dwc->usb3_phy);
>>>> +                    }
>>>> +            case 1:
>>>> +                    dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev,
>>>> +                            "usb-phy", 0);
>>>> +                    if (IS_ERR(dwc->usb2_phy)) {
>>>> +                            dev_err(dev, "usb2 phy not found\n");
>>>> +                            return PTR_ERR(dwc->usb2_phy);
>>>> +                    }
>>>> +                    break;
>>>
>>> In the Exynos case, there is only 1 phy and it is the USB3 phy. This code
>>> will wrongly treat it as usb2_phy.
>
> Thank you Roger for your concern regarding Exynos case.
> It's true that, Exynos5 series of SoCs have got only one IP block for
> DWC3'c PHY.
> This block is actually a combo of USB2 phy (UTMI+) as well as USB3 phy (PIPE3).
> And the same is served by a single USB phy driver. This is also
> clarified in the thread : https://lkml.org/lkml/2013/11/5/160
>
>>
>> That was the case even before this patch no? Unfortunately the old USB PHY
>> library doesn't have APIs to get PHYs in a better way. If we try modifying the
>> USB PHY library, it'll be kind of duplicating what is already there in the
>> Generic PHY library. I'd rather prefer Exynos guys to use the new framework.

we have tried moving Samsung's USB phy controller driver for DWC3, to
generic phy framework
so that things look clearer on Samsung's side too. The necessary
patches are availble at:
https://lkml.org/lkml/2013/10/31/79

>>
>> Thanks
>> Kishon
>> --
>> 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
>
>
>
> --
> Best Regards
> Vivek Gautam
> Samsung R&D Institute, Bangalore
> India



-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

Re: [PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Kishon Vijay Abraham I]

Hi Felipe,

On Thursday 17 October 2013 10:08 PM, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Oct 16, 2013 at 01:24:11AM +0530, Kishon Vijay Abraham I wrote:
>> There can be systems which does not have a external usb_phy, so get
>> usb_phy only if dt data indicates the presence of PHY in the case of dt boot or
>> if platform_data indicates the presence of PHY. Also remove checking if
>> return value is -ENXIO since it's now changed to always enable usb_phy layer.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> 
> I'm fine with this patch, but one comment below:
> 
>> @@ -387,16 +388,49 @@ static int dwc3_probe(struct platform_device *pdev)
>>  	if (node) {
>>  		dwc->maximum_speed = of_usb_get_maximum_speed(node);
>>  
>> -		dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
>> -		dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1);
>> +		count = of_count_phandle_with_args(node, "usb-phy", NULL);
>> +		switch (count) {
>> +		case 2:
>> +			dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev,
>> +				"usb-phy", 1);
>> +			if (IS_ERR(dwc->usb3_phy)) {
>> +				dev_err(dev, "usb3 phy not found\n");
>> +				return PTR_ERR(dwc->usb3_phy);
>> +			}
>> +		case 1:
>> +			dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev,
>> +				"usb-phy", 0);
>> +			if (IS_ERR(dwc->usb2_phy)) {
>> +				dev_err(dev, "usb2 phy not found\n");
>> +				return PTR_ERR(dwc->usb2_phy);
>> +			}
>> +			break;
>> +		default:
>> +			dev_err(dev, "usb phy nodes not configured\n");
>> +		}
>>  
>>  		dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize");
>>  		dwc->dr_mode = of_usb_get_dr_mode(node);
>>  	} else if (pdata) {
>>  		dwc->maximum_speed = pdata->maximum_speed;
>>  
>> -		dwc->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>> -		dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
>> +		if (pdata->usb2_phy) {
>> +			dwc->usb2_phy = devm_usb_get_phy(dev,
>> +				USB_PHY_TYPE_USB2);
>> +			if (IS_ERR(dwc->usb2_phy)) {
>> +				dev_err(dev, "usb2 phy not found\n");
>> +				return PTR_ERR(dwc->usb2_phy);
>> +			}
>> +		}
>> +
>> +		if (pdata->usb3_phy) {
>> +			dwc->usb3_phy = devm_usb_get_phy(dev,
>> +				USB_PHY_TYPE_USB3);
>> +			if (IS_ERR(dwc->usb3_phy)) {
>> +				dev_err(dev, "usb3 phy not found\n");
>> +				return PTR_ERR(dwc->usb3_phy);
>> +			}
>> +		}
>>  
>>  		dwc->needs_fifo_resize = pdata->tx_fifo_resize;
>>  		dwc->dr_mode = pdata->dr_mode;
>> @@ -409,36 +443,6 @@ static int dwc3_probe(struct platform_device *pdev)
>>  	if (dwc->maximum_speed == USB_SPEED_UNKNOWN)
>>  		dwc->maximum_speed = USB_SPEED_SUPER;
>>  
>> -	if (IS_ERR(dwc->usb2_phy)) {
>> -		ret = PTR_ERR(dwc->usb2_phy);
>> -
>> -		/*
>> -		 * if -ENXIO is returned, it means PHY layer wasn't
>> -		 * enabled, so it makes no sense to return -EPROBE_DEFER
>> -		 * in that case, since no PHY driver will ever probe.
>> -		 */
>> -		if (ret == -ENXIO)
>> -			return ret;
>> -
>> -		dev_err(dev, "no usb2 phy configured\n");
>> -		return -EPROBE_DEFER;
>> -	}
>> -
>> -	if (IS_ERR(dwc->usb3_phy)) {
>> -		ret = PTR_ERR(dwc->usb3_phy);
>> -
>> -		/*
>> -		 * if -ENXIO is returned, it means PHY layer wasn't
>> -		 * enabled, so it makes no sense to return -EPROBE_DEFER
>> -		 * in that case, since no PHY driver will ever probe.
>> -		 */
>> -		if (ret == -ENXIO)
>> -			return ret;
>> -
>> -		dev_err(dev, "no usb3 phy configured\n");
>> -		return -EPROBE_DEFER;
>> -	}
> 
> the idea for doing the error checking here was actually to avoid
> duplicating it in all previous cases, as you did. Please keep the error
> checking here and you're good to go.
> 
>> -
>>  	dwc->xhci_resources[0].start = res->start;
>>  	dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
>>  					DWC3_XHCI_REGS_END;
>> diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h
>> index 7db34f0..49ffa6d 100644
>> --- a/drivers/usb/dwc3/platform_data.h
>> +++ b/drivers/usb/dwc3/platform_data.h
>> @@ -24,4 +24,6 @@ struct dwc3_platform_data {
>>  	enum usb_device_speed maximum_speed;
>>  	enum usb_dr_mode dr_mode;
>>  	bool tx_fifo_resize;
>> +	bool usb2_phy;
>> +	bool usb3_phy;
> 
> This would look better as a quirks flag, then we could:
> 
> unsigned long quirks;
> #define DWC3_QUIRK_NO_USB3_PHY	BIT(0)
> #define DWC3_QUIRK_NO_USB2_PHY	BIT(1)

Should this quirk be used for dt also? Currently we find if it has usb3 phy or
usb2 phy from the dt data only. But if we add a quirk, we'll have to add a
property to populate the quirk no?

Thanks
Kishon

> 
> ...
> 
> if (!test_bit(DWC3_QUIRK_NO_USB3_PHY), &dwc->quirks &&
> 	dwc->has_usb3_phy) {
> 	grab_usb3_phy();
> }
> 
> cheers
> 

Re: [PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Felipe Balbi]

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

Hi,

On Mon, Nov 11, 2013 at 08:06:12PM +0530, Kishon Vijay Abraham I wrote:
> >> diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h
> >> index 7db34f0..49ffa6d 100644
> >> --- a/drivers/usb/dwc3/platform_data.h
> >> +++ b/drivers/usb/dwc3/platform_data.h
> >> @@ -24,4 +24,6 @@ struct dwc3_platform_data {
> >>  	enum usb_device_speed maximum_speed;
> >>  	enum usb_dr_mode dr_mode;
> >>  	bool tx_fifo_resize;
> >> +	bool usb2_phy;
> >> +	bool usb3_phy;
> > 
> > This would look better as a quirks flag, then we could:
> > 
> > unsigned long quirks;
> > #define DWC3_QUIRK_NO_USB3_PHY	BIT(0)
> > #define DWC3_QUIRK_NO_USB2_PHY	BIT(1)
> 
> Should this quirk be used for dt also? Currently we find if it has usb3 phy or
> usb2 phy from the dt data only. But if we add a quirk, we'll have to add a
> property to populate the quirk no?

either we use the quirk, or use the fact that no <&usb2_phy> phandle is
defined, would work both ways, no ?

-- 
balbi

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

Re: [PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Kishon Vijay Abraham I]

Hi,

On Tuesday 26 November 2013 03:02 AM, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Nov 11, 2013 at 08:06:12PM +0530, Kishon Vijay Abraham I wrote:
>>>> diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h
>>>> index 7db34f0..49ffa6d 100644
>>>> --- a/drivers/usb/dwc3/platform_data.h
>>>> +++ b/drivers/usb/dwc3/platform_data.h
>>>> @@ -24,4 +24,6 @@ struct dwc3_platform_data {
>>>>  	enum usb_device_speed maximum_speed;
>>>>  	enum usb_dr_mode dr_mode;
>>>>  	bool tx_fifo_resize;
>>>> +	bool usb2_phy;
>>>> +	bool usb3_phy;
>>>
>>> This would look better as a quirks flag, then we could:
>>>
>>> unsigned long quirks;
>>> #define DWC3_QUIRK_NO_USB3_PHY	BIT(0)
>>> #define DWC3_QUIRK_NO_USB2_PHY	BIT(1)
>>
>> Should this quirk be used for dt also? Currently we find if it has usb3 phy or
>> usb2 phy from the dt data only. But if we add a quirk, we'll have to add a
>> property to populate the quirk no?
> 
> either we use the quirk, or use the fact that no <&usb2_phy> phandle is
> defined, would work both ways, no ?

In my v3, I've made both to use quirks since we don't want to have separate
mechanism for dt and non-dt stuff to know the presence of a particular PHY.

Thanks
Kishon

Re: [PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Heikki Krogerus]

Hi,

On Thu, Oct 17, 2013 at 09:54:26AM -0500, Felipe Balbi wrote:
> On Wed, Oct 16, 2013 at 04:27:26PM +0300, Roger Quadros wrote:
> > On 10/16/2013 04:10 PM, Kishon Vijay Abraham I wrote:
> > Do you know if there are users of dwc3 other than exynos5250 and omap5?
> > If not, we should get rid of the old USB PHY altogether.
> 
> Intel's Baytrail, at least. But they don't use DT.

I don't see any use for the generic-phy when the device is enumerated
from PCI. If dwc3 can live without phys, there should not be any
problem dropping the old USB PHY support.

Br,

-- 
heikki

Re: [PATCH v2 2/7] usb: dwc3: adapt dwc3 core to use Generic PHY Framework

[Heikki Krogerus]

Hi Kishon,

On Wed, Oct 16, 2013 at 01:24:12AM +0530, Kishon Vijay Abraham I wrote:
> +	count = of_property_match_string(node, "phy-names", "usb2-phy");
> +	if (count >= 0 || (pdata && pdata->usb2_generic_phy)) {
> +		dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> +		if (IS_ERR(dwc->usb2_generic_phy)) {
> +			dev_err(dev, "no usb2 phy configured yet");
> +			return PTR_ERR(dwc->usb2_generic_phy);
> +		}
> +		dwc->usb2_phy = NULL;
> +	}
> +
> +	count = of_property_match_string(node, "phy-names", "usb3-phy");
> +	if (count >= 0 || (pdata && pdata->usb3_generic_phy)) {
> +		dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> +		if (IS_ERR(dwc->usb3_generic_phy)) {
> +			dev_err(dev, "no usb3 phy configured yet");
> +			return PTR_ERR(dwc->usb3_generic_phy);
> +		}
> +		dwc->usb3_phy = NULL;
> +	}

Is there some specific reason for these checks? The driver should not
need to care about the platform (DT, ACPI, platform based).

Just get the phys and check the return value. In case ERR_PTR(-ENODEV)
leave the phy as NULL and let the driver continue normally. With other
errors you make the dwc3 probe fail.

Thanks,

-- 
heikki

Re: [PATCH v2 2/7] usb: dwc3: adapt dwc3 core to use Generic PHY Framework

[Kishon Vijay Abraham I]

Hi,

On Tuesday 03 December 2013 05:29 PM, Heikki Krogerus wrote:
> Hi Kishon,
> 
> On Wed, Oct 16, 2013 at 01:24:12AM +0530, Kishon Vijay Abraham I wrote:
>> +	count = of_property_match_string(node, "phy-names", "usb2-phy");
>> +	if (count >= 0 || (pdata && pdata->usb2_generic_phy)) {
>> +		dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
>> +		if (IS_ERR(dwc->usb2_generic_phy)) {
>> +			dev_err(dev, "no usb2 phy configured yet");
>> +			return PTR_ERR(dwc->usb2_generic_phy);
>> +		}
>> +		dwc->usb2_phy = NULL;
>> +	}
>> +
>> +	count = of_property_match_string(node, "phy-names", "usb3-phy");
>> +	if (count >= 0 || (pdata && pdata->usb3_generic_phy)) {
>> +		dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>> +		if (IS_ERR(dwc->usb3_generic_phy)) {
>> +			dev_err(dev, "no usb3 phy configured yet");
>> +			return PTR_ERR(dwc->usb3_generic_phy);
>> +		}
>> +		dwc->usb3_phy = NULL;
>> +	}
> 
> Is there some specific reason for these checks? The driver should not
> need to care about the platform (DT, ACPI, platform based).

yeah just wanted to throw an error if a platform needs PHY but wasn't able to
get it. Btw this has changed after my v3 of this patch series which I sent
sometime back [1] where we use quirks to know if a PHY is needed for that
platform or not.

http://www.spinics.net/lists/linux-usb/msg98077.html

Thanks
Kishon

Re: [PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Felipe Balbi]

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

On Tue, Dec 03, 2013 at 12:39:50PM +0200, Heikki Krogerus wrote:
> Hi,
> 
> On Thu, Oct 17, 2013 at 09:54:26AM -0500, Felipe Balbi wrote:
> > On Wed, Oct 16, 2013 at 04:27:26PM +0300, Roger Quadros wrote:
> > > On 10/16/2013 04:10 PM, Kishon Vijay Abraham I wrote:
> > > Do you know if there are users of dwc3 other than exynos5250 and omap5?
> > > If not, we should get rid of the old USB PHY altogether.
> > 
> > Intel's Baytrail, at least. But they don't use DT.
> 
> I don't see any use for the generic-phy when the device is enumerated
> from PCI. If dwc3 can live without phys, there should not be any
> problem dropping the old USB PHY support.

yeah, I don't want to drop PHY support, I want to require everybody to
provide a PHY, otherwise we have too many options to handle and that's
never clean.

-- 
balbi

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

Re: [PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Heikki Krogerus]

Hi,

On Fri, Dec 06, 2013 at 02:15:30PM -0600, Felipe Balbi wrote:
> On Tue, Dec 03, 2013 at 12:39:50PM +0200, Heikki Krogerus wrote:
> > On Thu, Oct 17, 2013 at 09:54:26AM -0500, Felipe Balbi wrote:
> > > On Wed, Oct 16, 2013 at 04:27:26PM +0300, Roger Quadros wrote:
> > > > On 10/16/2013 04:10 PM, Kishon Vijay Abraham I wrote:
> > > > Do you know if there are users of dwc3 other than exynos5250 and omap5?
> > > > If not, we should get rid of the old USB PHY altogether.
> > > 
> > > Intel's Baytrail, at least. But they don't use DT.
> > 
> > I don't see any use for the generic-phy when the device is enumerated
> > from PCI. If dwc3 can live without phys, there should not be any
> > problem dropping the old USB PHY support.
> 
> yeah, I don't want to drop PHY support, I want to require everybody to
> provide a PHY, otherwise we have too many options to handle and that's
> never clean.

I have to clarify in case there was a misunderstanding. When I said
generic-phy I meant drivers/usb/phy/phy-generic.c and I was not
talking about Kishon's new generic PHY framework.

So I was only saying that if the dwc3-pci.c is the only thing that
needs the old usb phy support, then there should not be any problem
dropping the support for it.

Thanks,

-- 
heikki

Re: [PATCH v2 1/7] usb: dwc3: get "usb_phy" only if the platform indicates the presence of PHY's

[Felipe Balbi]

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

On Mon, Dec 09, 2013 at 10:52:45AM +0200, Heikki Krogerus wrote:
> Hi,
> 
> On Fri, Dec 06, 2013 at 02:15:30PM -0600, Felipe Balbi wrote:
> > On Tue, Dec 03, 2013 at 12:39:50PM +0200, Heikki Krogerus wrote:
> > > On Thu, Oct 17, 2013 at 09:54:26AM -0500, Felipe Balbi wrote:
> > > > On Wed, Oct 16, 2013 at 04:27:26PM +0300, Roger Quadros wrote:
> > > > > On 10/16/2013 04:10 PM, Kishon Vijay Abraham I wrote:
> > > > > Do you know if there are users of dwc3 other than exynos5250 and omap5?
> > > > > If not, we should get rid of the old USB PHY altogether.
> > > > 
> > > > Intel's Baytrail, at least. But they don't use DT.
> > > 
> > > I don't see any use for the generic-phy when the device is enumerated
> > > from PCI. If dwc3 can live without phys, there should not be any
> > > problem dropping the old USB PHY support.
> > 
> > yeah, I don't want to drop PHY support, I want to require everybody to
> > provide a PHY, otherwise we have too many options to handle and that's
> > never clean.
> 
> I have to clarify in case there was a misunderstanding. When I said
> generic-phy I meant drivers/usb/phy/phy-generic.c and I was not
> talking about Kishon's new generic PHY framework.
> 
> So I was only saying that if the dwc3-pci.c is the only thing that
> needs the old usb phy support, then there should not be any problem
> dropping the support for it.

oh, ok. Got it now, thanks.

-- 
balbi

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