[PATCH v3 00/10] Make dwc3 use Generic PHY Framework

[PATCH v3 00/10] Make dwc3 use Generic PHY Framework

[Kishon Vijay Abraham I]

Modified dwc3 core to find PHYs only if the platform indicates that it has 
to use a PHY (using quirks). 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 v2:
* added a couple of fixes. One is invoking phy_resume after phy_init and the
other is power off phy in error patch
* used quirks to identify if a particular platform does not have PHYs
* removed using separate header for pipe3 driver and also removed all referencs
to SATA and PCIe in pipe3 driver since it's not yet adapted for those drivers.

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 (10):
  usb: dwc3: invoke phy_resume after phy_init
  usb: dwc3: power off usb phy in error path
  usb: dwc3: preparation for adapting dwc3 to generic phy framework
  usb: dwc3: use quirks to know if a particualr platform doesn't have
    PHY
  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}   |    8 +-
 Documentation/devicetree/bindings/usb/dwc3.txt     |    6 +-
 arch/arm/boot/dts/omap5.dtsi                       |    4 +
 drivers/phy/Kconfig                                |   11 +
 drivers/phy/Makefile                               |    1 +
 drivers/phy/phy-omap-usb2.c                        |   27 +--
 .../phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c}     |  232 ++++++++++++--------
 drivers/usb/dwc3/Kconfig                           |    2 +
 drivers/usb/dwc3/core.c                            |  189 +++++++++++-----
 drivers/usb/dwc3/core.h                            |   22 ++
 drivers/usb/dwc3/platform_data.h                   |    5 +
 drivers/usb/phy/Kconfig                            |   11 -
 drivers/usb/phy/Makefile                           |    1 -
 include/linux/{usb => phy}/omap_usb.h              |    3 -
 14 files changed, 320 insertions(+), 202 deletions(-)
 rename Documentation/devicetree/bindings/{usb/usb-phy.txt => phy/ti-phy.txt} (88%)
 rename drivers/{usb/phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c} (55%)
 rename include/linux/{usb => phy}/omap_usb.h (95%)

-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 01/10] usb: dwc3: invoke phy_resume after phy_init

[Kishon Vijay Abraham I]

usb_phy_set_suspend(phy, 0) is called before usb_phy_init. Fix it.

Reported-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/usb/dwc3/core.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 74f9cf0..4dd1f7e 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -455,9 +455,6 @@ static int dwc3_probe(struct platform_device *pdev)
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
 
-	usb_phy_set_suspend(dwc->usb2_phy, 0);
-	usb_phy_set_suspend(dwc->usb3_phy, 0);
-
 	spin_lock_init(&dwc->lock);
 	platform_set_drvdata(pdev, dwc);
 
@@ -488,6 +485,9 @@ static int dwc3_probe(struct platform_device *pdev)
 		goto err0;
 	}
 
+	usb_phy_set_suspend(dwc->usb2_phy, 0);
+	usb_phy_set_suspend(dwc->usb3_phy, 0);
+
 	ret = dwc3_event_buffers_setup(dwc);
 	if (ret) {
 		dev_err(dwc->dev, "failed to setup event buffers\n");
-- 
1.7.10.4

[PATCH v3 02/10] usb: dwc3: power off usb phy in error path

[Kishon Vijay Abraham I]

usb phy was power'ed on but was never power'ed off in the error path.
Fix it.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/usb/dwc3/core.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 4dd1f7e..a49217a 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -569,6 +569,8 @@ err2:
 	dwc3_event_buffers_cleanup(dwc);
 
 err1:
+	usb_phy_set_suspend(dwc->usb2_phy, 1);
+	usb_phy_set_suspend(dwc->usb3_phy, 1);
 	dwc3_core_exit(dwc);
 
 err0:
-- 
1.7.10.4

[PATCH v3 03/10] usb: dwc3: preparation for adapting dwc3 to generic phy framework

[Kishon Vijay Abraham I]

Invoke usb_phy api's only if usb_phy does not have error value. This is
needed for both the legacy usb_phy and generic phy to co-exist in dwc3 core.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/usb/dwc3/core.c |   42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index a49217a..986674f 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -80,8 +80,10 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
 	reg |= DWC3_GUSB2PHYCFG_PHYSOFTRST;
 	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
 
-	usb_phy_init(dwc->usb2_phy);
-	usb_phy_init(dwc->usb3_phy);
+	if (!IS_ERR(dwc->usb2_phy))
+		usb_phy_init(dwc->usb2_phy);
+	if (!IS_ERR(dwc->usb3_phy))
+		usb_phy_init(dwc->usb3_phy);
 	mdelay(100);
 
 	/* Clear USB3 PHY reset */
@@ -341,8 +343,10 @@ err0:
 
 static void dwc3_core_exit(struct dwc3 *dwc)
 {
-	usb_phy_shutdown(dwc->usb2_phy);
-	usb_phy_shutdown(dwc->usb3_phy);
+	if (!IS_ERR(dwc->usb2_phy))
+		usb_phy_shutdown(dwc->usb2_phy);
+	if (!IS_ERR(dwc->usb3_phy))
+		usb_phy_shutdown(dwc->usb3_phy);
 }
 
 #define DWC3_ALIGN_MASK		(16 - 1)
@@ -485,8 +489,10 @@ static int dwc3_probe(struct platform_device *pdev)
 		goto err0;
 	}
 
-	usb_phy_set_suspend(dwc->usb2_phy, 0);
-	usb_phy_set_suspend(dwc->usb3_phy, 0);
+	if (!IS_ERR(dwc->usb2_phy))
+		usb_phy_set_suspend(dwc->usb2_phy, 0);
+	if (!IS_ERR(dwc->usb3_phy))
+		usb_phy_set_suspend(dwc->usb3_phy, 0);
 
 	ret = dwc3_event_buffers_setup(dwc);
 	if (ret) {
@@ -569,8 +575,10 @@ err2:
 	dwc3_event_buffers_cleanup(dwc);
 
 err1:
-	usb_phy_set_suspend(dwc->usb2_phy, 1);
-	usb_phy_set_suspend(dwc->usb3_phy, 1);
+	if (!IS_ERR(dwc->usb2_phy))
+		usb_phy_set_suspend(dwc->usb2_phy, 1);
+	if (!IS_ERR(dwc->usb3_phy))
+		usb_phy_set_suspend(dwc->usb3_phy, 1);
 	dwc3_core_exit(dwc);
 
 err0:
@@ -583,8 +591,10 @@ static int dwc3_remove(struct platform_device *pdev)
 {
 	struct dwc3	*dwc = platform_get_drvdata(pdev);
 
-	usb_phy_set_suspend(dwc->usb2_phy, 1);
-	usb_phy_set_suspend(dwc->usb3_phy, 1);
+	if (!IS_ERR(dwc->usb2_phy))
+		usb_phy_set_suspend(dwc->usb2_phy, 1);
+	if (!IS_ERR(dwc->usb3_phy))
+		usb_phy_set_suspend(dwc->usb3_phy, 1);
 
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
@@ -680,8 +690,10 @@ static int dwc3_suspend(struct device *dev)
 	dwc->gctl = dwc3_readl(dwc->regs, DWC3_GCTL);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
-	usb_phy_shutdown(dwc->usb3_phy);
-	usb_phy_shutdown(dwc->usb2_phy);
+	if (!IS_ERR(dwc->usb3_phy))
+		usb_phy_shutdown(dwc->usb3_phy);
+	if (!IS_ERR(dwc->usb2_phy))
+		usb_phy_shutdown(dwc->usb2_phy);
 
 	return 0;
 }
@@ -691,8 +703,10 @@ static int dwc3_resume(struct device *dev)
 	struct dwc3	*dwc = dev_get_drvdata(dev);
 	unsigned long	flags;
 
-	usb_phy_init(dwc->usb3_phy);
-	usb_phy_init(dwc->usb2_phy);
+	if (!IS_ERR(dwc->usb3_phy))
+		usb_phy_init(dwc->usb3_phy);
+	if (!IS_ERR(dwc->usb2_phy))
+		usb_phy_init(dwc->usb2_phy);
 
 	spin_lock_irqsave(&dwc->lock, flags);
 
-- 
1.7.10.4

[PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY

[Kishon Vijay Abraham I]

There can be systems which does not have an external phy, so get
phy only if no quirks are added that indicates the PHY is not present.
Introduced two quirk flags to indicate the *absence* of usb2 phy and
usb3 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>
---
 drivers/usb/dwc3/Kconfig         |    1 +
 drivers/usb/dwc3/core.c          |   93 ++++++++++++++++++++------------------
 drivers/usb/dwc3/core.h          |   15 ++++++
 drivers/usb/dwc3/platform_data.h |    5 ++
 4 files changed, 69 insertions(+), 45 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 986674f..f20f4a1 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -32,6 +32,7 @@
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
@@ -351,13 +352,28 @@ static void dwc3_core_exit(struct dwc3 *dwc)
 
 #define DWC3_ALIGN_MASK		(16 - 1)
 
+#ifdef CONFIG_OF
+static const struct of_device_id of_dwc3_match[] = {
+	{
+		.compatible = "snps,dwc3"
+	},
+	{
+		.compatible = "synopsys,dwc3"
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_dwc3_match);
+#endif
+
 static int dwc3_probe(struct platform_device *pdev)
 {
 	struct device		*dev = &pdev->dev;
 	struct dwc3_platform_data *pdata = dev_get_platdata(dev);
 	struct device_node	*node = dev->of_node;
+	const struct of_device_id *of_id;
 	struct resource		*res;
 	struct dwc3		*dwc;
+	struct dwc3_data	*dwc_data;
 
 	int			ret = -ENOMEM;
 
@@ -391,16 +407,36 @@ 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);
+		of_id = of_match_device(of_match_ptr(of_dwc3_match),
+			&pdev->dev);
+		if (!of_id)
+			return -EINVAL;
+
+		dwc_data = (struct dwc3_data *)of_id->data;
+
+		if (dwc_data)
+			dwc->quirks = dwc_data->quirks;
+
+		if (!(dwc->quirks & DWC3_QUIRK_NO_USB2_PHY))
+			dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev,
+				"usb-phy", 0);
+
+		if (!(dwc->quirks & DWC3_QUIRK_NO_USB3_PHY))
+			dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev,
+				"usb-phy", 1);
 
 		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->quirks & DWC3_QUIRK_NO_USB2_PHY))
+			dwc->usb2_phy = devm_usb_get_phy(dev,
+				USB_PHY_TYPE_USB2);
+
+		if (!(pdata->quirks & DWC3_QUIRK_NO_USB3_PHY))
+			dwc->usb3_phy = devm_usb_get_phy(dev,
+				USB_PHY_TYPE_USB3);
 
 		dwc->needs_fifo_resize = pdata->tx_fifo_resize;
 		dwc->dr_mode = pdata->dr_mode;
@@ -409,40 +445,20 @@ static int dwc3_probe(struct platform_device *pdev)
 		dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
 	}
 
-	/* default to superspeed if no maximum_speed passed */
-	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;
+		dev_err(dev, "usb2 phy not found\n");
+		return PTR_ERR(dwc->usb2_phy);
 	}
 
 	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;
+		dev_err(dev, "usb3 phy not found\n");
+		return PTR_ERR(dwc->usb3_phy);
 	}
 
+	/* default to superspeed if no maximum_speed passed */
+	if (dwc->maximum_speed == USB_SPEED_UNKNOWN)
+		dwc->maximum_speed = USB_SPEED_SUPER;
+
 	dwc->xhci_resources[0].start = res->start;
 	dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
 					DWC3_XHCI_REGS_END;
@@ -744,19 +760,6 @@ static const struct dev_pm_ops dwc3_dev_pm_ops = {
 #define DWC3_PM_OPS	NULL
 #endif
 
-#ifdef CONFIG_OF
-static const struct of_device_id of_dwc3_match[] = {
-	{
-		.compatible = "snps,dwc3"
-	},
-	{
-		.compatible = "synopsys,dwc3"
-	},
-	{ },
-};
-MODULE_DEVICE_TABLE(of, of_dwc3_match);
-#endif
-
 static struct platform_driver dwc3_driver = {
 	.probe		= dwc3_probe,
 	.remove		= dwc3_remove,
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index f8af8d4..1e11dd1 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -36,6 +36,10 @@
 #define DWC3_ENDPOINTS_NUM	32
 #define DWC3_XHCI_RESOURCES_NUM	2
 
+/* quirk flags */
+#define DWC3_QUIRK_NO_USB2_PHY	(0x1 << 0)
+#define DWC3_QUIRK_NO_USB3_PHY	(0x1 << 1)
+
 #define DWC3_EVENT_SIZE		4	/* bytes */
 #define DWC3_EVENT_MAX_NUM	64	/* 2 events/endpoint */
 #define DWC3_EVENT_BUFFERS_SIZE	(DWC3_EVENT_SIZE * DWC3_EVENT_MAX_NUM)
@@ -637,6 +641,7 @@ struct dwc3_scratchpad_array {
  * @mem: points to start of memory which is used for this struct.
  * @hwparams: copy of hwparams registers
  * @root: debugfs root folder pointer
+ * @quirks: attributes that are unique to a specific device (like erratas)
  */
 struct dwc3 {
 	struct usb_ctrlrequest	*ctrl_req;
@@ -730,6 +735,8 @@ struct dwc3 {
 
 	u8			test_mode;
 	u8			test_mode_nr;
+
+	u32			quirks;
 };
 
 /* -------------------------------------------------------------------------- */
@@ -856,6 +863,14 @@ union dwc3_event {
 	struct dwc3_event_gevt		gevt;
 };
 
+/**
+ * dwc3_data - dwc3 driver data
+ * @quirks - quirks specific to a device
+ */
+struct dwc3_data {
+	u32	quirks;
+};
+
 /*
  * DWC3 Features to be used as Driver Data
  */
diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h
index 7db34f0..4179980 100644
--- a/drivers/usb/dwc3/platform_data.h
+++ b/drivers/usb/dwc3/platform_data.h
@@ -20,8 +20,13 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/otg.h>
 
+/* quirk flags */
+#define DWC3_QUIRK_NO_USB2_PHY (0x1 << 0)
+#define DWC3_QUIRK_NO_USB3_PHY (0x1 << 1)
+
 struct dwc3_platform_data {
 	enum usb_device_speed maximum_speed;
 	enum usb_dr_mode dr_mode;
 	bool tx_fifo_resize;
+	u32 quirks;
 };
-- 
1.7.10.4

Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY

[Felipe Balbi]

[-- Attachment #1.1: Type: text/plain, Size: 2445 bytes --]

On Mon, Nov 25, 2013 at 03:31:24PM +0530, Kishon Vijay Abraham I wrote:
> There can be systems which does not have an external phy, so get
> phy only if no quirks are added that indicates the PHY is not present.
> Introduced two quirk flags to indicate the *absence* of usb2 phy and
> usb3 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>
> ---
>  drivers/usb/dwc3/Kconfig         |    1 +
>  drivers/usb/dwc3/core.c          |   93 ++++++++++++++++++++------------------
>  drivers/usb/dwc3/core.h          |   15 ++++++
>  drivers/usb/dwc3/platform_data.h |    5 ++
>  4 files changed, 69 insertions(+), 45 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

no more selects, we've already had too many problems caused by bogus
usage of select.

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 986674f..f20f4a1 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -32,6 +32,7 @@
>  #include <linux/delay.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/gadget.h>
> @@ -351,13 +352,28 @@ static void dwc3_core_exit(struct dwc3 *dwc)
>  
>  #define DWC3_ALIGN_MASK		(16 - 1)
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_dwc3_match[] = {
> +	{
> +		.compatible = "snps,dwc3"
> +	},
> +	{
> +		.compatible = "synopsys,dwc3"
> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, of_dwc3_match);
> +#endif
> +

you can split this patch a little bit just to make it easier to review
because there are a few changes which are really obvious:

a) move of_device_id higher in source code so you can use it from probe.
	while at that, aso use of_match_device() and return early if it
	doesn't match anything.

b) introduce struct dwc3_data.

c) initialize of_id->data field.

d) start using struct dwc3_data

this will make to put all obvious changes (a-c) in separate patches so
review can focus on (d).

-- 
balbi

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY

[Heikki Krogerus]

Hi guys,

Kishon, sorry I did not see this v3 set.

On Mon, Nov 25, 2013 at 03:31:24PM +0530, Kishon Vijay Abraham I wrote:
> There can be systems which does not have an external phy, so get
> phy only if no quirks are added that indicates the PHY is not present.
> Introduced two quirk flags to indicate the *absence* of usb2 phy and
> usb3 phy. Also remove checking if return value is -ENXIO since it's now
> changed to always enable usb_phy layer.

Can you guys explain why is something like this needed? Like with
clocks and gpios, the device drivers shouldn't need to care any more
if the platform has the phys or not. -ENODEV tells you your platform
does not have them, so go ahead and continue when getting the phy.

I just tested removing the phy registering from dwc3-pci.c and all I
had to do was to add IS_ERR(<phy>) checks where the old usb phys were
used and everything worked fine?

What am I missing?

Br,

-- 
heikki

Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY

[Kishon Vijay Abraham I]

Hi,

On Wednesday 04 December 2013 08:10 PM, Heikki Krogerus wrote:
> Hi guys,
> 
> Kishon, sorry I did not see this v3 set.
> 
> On Mon, Nov 25, 2013 at 03:31:24PM +0530, Kishon Vijay Abraham I wrote:
>> There can be systems which does not have an external phy, so get
>> phy only if no quirks are added that indicates the PHY is not present.
>> Introduced two quirk flags to indicate the *absence* of usb2 phy and
>> usb3 phy. Also remove checking if return value is -ENXIO since it's now
>> changed to always enable usb_phy layer.
> 
> Can you guys explain why is something like this needed? Like with
> clocks and gpios, the device drivers shouldn't need to care any more
> if the platform has the phys or not. -ENODEV tells you your platform

Shouldn't we report if a particular platform needs a PHY and not able to get
it. How will a user know if a particular controller is not working because it's
not able to get and initialize the PHYs? Don't you think in such cases it's
better to fail (and return from probe) because the controller will not work
anyway without the PHY?

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY

[Heikki Krogerus]

Hi,

On Thu, Dec 05, 2013 at 12:04:46PM +0530, Kishon Vijay Abraham I wrote:
> On Wednesday 04 December 2013 08:10 PM, Heikki Krogerus wrote:
> > On Mon, Nov 25, 2013 at 03:31:24PM +0530, Kishon Vijay Abraham I wrote:
> >> There can be systems which does not have an external phy, so get
> >> phy only if no quirks are added that indicates the PHY is not present.
> >> Introduced two quirk flags to indicate the *absence* of usb2 phy and
> >> usb3 phy. Also remove checking if return value is -ENXIO since it's now
> >> changed to always enable usb_phy layer.
> > 
> > Can you guys explain why is something like this needed? Like with
> > clocks and gpios, the device drivers shouldn't need to care any more
> > if the platform has the phys or not. -ENODEV tells you your platform
> 
> Shouldn't we report if a particular platform needs a PHY and not able to get
> it. How will a user know if a particular controller is not working because it's
> not able to get and initialize the PHYs? Don't you think in such cases it's
> better to fail (and return from probe) because the controller will not work
> anyway without the PHY?

My point is that you do not need to separately tell this to the driver
like you do with the quirks (if you did, then you would need to fix
your framework and not hack the drivers).

Like I said, ENODEV tells you that there is no phy on this platform
for you, allowing you to safely continue. If your phy driver is not
loaded, the framework already returns EPROBE_DEFER, right. Any other
error when getting the phy you can consider critical. They are the
errors telling you that you do need a phy on this platform, but
something actually went wrong when getting it.

Those "quirks" should always be avoided, and I don't see any use for
them here.

> Thanks
> Kishon

Thanks,

-- 
heikki

Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY

[Kishon Vijay Abraham I]

Hi,

On Thursday 05 December 2013 01:28 PM, Heikki Krogerus wrote:
> Hi,
>
> On Thu, Dec 05, 2013 at 12:04:46PM +0530, Kishon Vijay Abraham I wrote:
>> On Wednesday 04 December 2013 08:10 PM, Heikki Krogerus wrote:
>>> On Mon, Nov 25, 2013 at 03:31:24PM +0530, Kishon Vijay Abraham I wrote:
>>>> There can be systems which does not have an external phy, so get
>>>> phy only if no quirks are added that indicates the PHY is not present.
>>>> Introduced two quirk flags to indicate the *absence* of usb2 phy and
>>>> usb3 phy. Also remove checking if return value is -ENXIO since it's now
>>>> changed to always enable usb_phy layer.
>>>
>>> Can you guys explain why is something like this needed? Like with
>>> clocks and gpios, the device drivers shouldn't need to care any more
>>> if the platform has the phys or not. -ENODEV tells you your platform
>>
>> Shouldn't we report if a particular platform needs a PHY and not able to get
>> it. How will a user know if a particular controller is not working because it's
>> not able to get and initialize the PHYs? Don't you think in such cases it's
>> better to fail (and return from probe) because the controller will not work
>> anyway without the PHY?
>
> My point is that you do not need to separately tell this to the driver
> like you do with the quirks (if you did, then you would need to fix
> your framework and not hack the drivers).
>
> Like I said, ENODEV tells you that there is no phy on this platform
> for you, allowing you to safely continue. If your phy driver is not
> loaded, the framework already returns EPROBE_DEFER, right. Any other

right. but that doesn't consider broken dt data. With quirks we'll able 
to tell if a controller in a particular platform has PHY or not without 
depending on the dt data.
> error when getting the phy you can consider critical. They are the
> errors telling you that you do need a phy on this platform, but
> something actually went wrong when getting it.
Not on all scenarios though :-s

Thanks
Kishon

Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY

[Heikki Krogerus]

Hi,

On Mon, Dec 09, 2013 at 12:43:37PM +0530, Kishon Vijay Abraham I wrote:
> On Thursday 05 December 2013 01:28 PM, Heikki Krogerus wrote:
> >On Thu, Dec 05, 2013 at 12:04:46PM +0530, Kishon Vijay Abraham I wrote:
> >>On Wednesday 04 December 2013 08:10 PM, Heikki Krogerus wrote:
> >>>On Mon, Nov 25, 2013 at 03:31:24PM +0530, Kishon Vijay Abraham I wrote:
> >>>>There can be systems which does not have an external phy, so get
> >>>>phy only if no quirks are added that indicates the PHY is not present.
> >>>>Introduced two quirk flags to indicate the *absence* of usb2 phy and
> >>>>usb3 phy. Also remove checking if return value is -ENXIO since it's now
> >>>>changed to always enable usb_phy layer.
> >>>
> >>>Can you guys explain why is something like this needed? Like with
> >>>clocks and gpios, the device drivers shouldn't need to care any more
> >>>if the platform has the phys or not. -ENODEV tells you your platform
> >>
> >>Shouldn't we report if a particular platform needs a PHY and not able to get
> >>it. How will a user know if a particular controller is not working because it's
> >>not able to get and initialize the PHYs? Don't you think in such cases it's
> >>better to fail (and return from probe) because the controller will not work
> >>anyway without the PHY?
> >
> >My point is that you do not need to separately tell this to the driver
> >like you do with the quirks (if you did, then you would need to fix
> >your framework and not hack the drivers).
> >
> >Like I said, ENODEV tells you that there is no phy on this platform
> >for you, allowing you to safely continue. If your phy driver is not
> >loaded, the framework already returns EPROBE_DEFER, right. Any other
> 
> right. but that doesn't consider broken dt data. With quirks we'll
> able to tell if a controller in a particular platform has PHY or not
> without depending on the dt data.

Broken dt data? What kind of scenario are you thinking here? Do you
mean case where the dt does not describe the phy on a platform that
depends on it? Shouldn't that problem be fixed in the dt and not
hacked in the drivers? Or are you thinking about something else?

Is there a case where something like that is actually happening?

Br,

-- 
heikki

Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY

[Heikki Krogerus]

Hi,

On Mon, Dec 09, 2013 at 11:26:04AM +0200, Heikki Krogerus wrote:
> > >>>Can you guys explain why is something like this needed? Like with
> > >>>clocks and gpios, the device drivers shouldn't need to care any more
> > >>>if the platform has the phys or not. -ENODEV tells you your platform
> > >>
> > >>Shouldn't we report if a particular platform needs a PHY and not able to get
> > >>it. How will a user know if a particular controller is not working because it's
> > >>not able to get and initialize the PHYs? Don't you think in such cases it's
> > >>better to fail (and return from probe) because the controller will not work
> > >>anyway without the PHY?
> > >
> > >My point is that you do not need to separately tell this to the driver
> > >like you do with the quirks (if you did, then you would need to fix
> > >your framework and not hack the drivers).
> > >
> > >Like I said, ENODEV tells you that there is no phy on this platform
> > >for you, allowing you to safely continue. If your phy driver is not
> > >loaded, the framework already returns EPROBE_DEFER, right. Any other
> > 
> > right. but that doesn't consider broken dt data. With quirks we'll
> > able to tell if a controller in a particular platform has PHY or not
> > without depending on the dt data.
> 
> Broken dt data? What kind of scenario are you thinking here? Do you
> mean case where the dt does not describe the phy on a platform that
> depends on it? Shouldn't that problem be fixed in the dt and not
> hacked in the drivers? Or are you thinking about something else?
> 
> Is there a case where something like that is actually happening?

I'm guessing I'm not getting an answer to this one.

Look, this patch will not work with ACPI enumerated devices. We will
have a platform providing a single ACPI id, but there is a whole bunch
of boards based on it and we have no way of telling which of them
need/have phys to deal with and which ones don't.


Thanks,

-- 
heikki

Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY

[Kishon Vijay Abraham I]

On Wednesday 11 December 2013 02:23 PM, Heikki Krogerus wrote:
> Hi,
> 
> On Mon, Dec 09, 2013 at 11:26:04AM +0200, Heikki Krogerus wrote:
>>>>>> Can you guys explain why is something like this needed? Like with
>>>>>> clocks and gpios, the device drivers shouldn't need to care any more
>>>>>> if the platform has the phys or not. -ENODEV tells you your platform
>>>>>
>>>>> Shouldn't we report if a particular platform needs a PHY and not able to get
>>>>> it. How will a user know if a particular controller is not working because it's
>>>>> not able to get and initialize the PHYs? Don't you think in such cases it's
>>>>> better to fail (and return from probe) because the controller will not work
>>>>> anyway without the PHY?
>>>>
>>>> My point is that you do not need to separately tell this to the driver
>>>> like you do with the quirks (if you did, then you would need to fix
>>>> your framework and not hack the drivers).
>>>>
>>>> Like I said, ENODEV tells you that there is no phy on this platform
>>>> for you, allowing you to safely continue. If your phy driver is not
>>>> loaded, the framework already returns EPROBE_DEFER, right. Any other
>>>
>>> right. but that doesn't consider broken dt data. With quirks we'll
>>> able to tell if a controller in a particular platform has PHY or not
>>> without depending on the dt data.
>>
>> Broken dt data? What kind of scenario are you thinking here? Do you
>> mean case where the dt does not describe the phy on a platform that
>> depends on it? Shouldn't that problem be fixed in the dt and not
>> hacked in the drivers? Or are you thinking about something else?
>>
>> Is there a case where something like that is actually happening?
> 
> I'm guessing I'm not getting an answer to this one.
> 
> Look, this patch will not work with ACPI enumerated devices. We will
> have a platform providing a single ACPI id, but there is a whole bunch
> of boards based on it and we have no way of telling which of them
> need/have phys to deal with and which ones don't.

Alright.. I'll drop this patch then.

Thanks
Kishon

[PATCH v3 05/10] 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                        |   66 +++++++++++++++++++++---
 drivers/usb/dwc3/core.h                        |    7 +++
 4 files changed, 70 insertions(+), 10 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 f20f4a1..628b372 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -85,6 +85,12 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
 		usb_phy_init(dwc->usb2_phy);
 	if (!IS_ERR(dwc->usb3_phy))
 		usb_phy_init(dwc->usb3_phy);
+
+	if (!IS_ERR(dwc->usb2_generic_phy))
+		phy_init(dwc->usb2_generic_phy);
+	if (!IS_ERR(dwc->usb3_generic_phy))
+		phy_init(dwc->usb3_generic_phy);
+
 	mdelay(100);
 
 	/* Clear USB3 PHY reset */
@@ -348,6 +354,12 @@ static void dwc3_core_exit(struct dwc3 *dwc)
 		usb_phy_shutdown(dwc->usb2_phy);
 	if (!IS_ERR(dwc->usb3_phy))
 		usb_phy_shutdown(dwc->usb3_phy);
+
+	if (!IS_ERR(dwc->usb2_generic_phy))
+		phy_power_off(dwc->usb2_generic_phy);
+	if (!IS_ERR(dwc->usb3_generic_phy))
+		phy_power_off(dwc->usb3_generic_phy);
+
 }
 
 #define DWC3_ALIGN_MASK		(16 - 1)
@@ -417,26 +429,34 @@ static int dwc3_probe(struct platform_device *pdev)
 		if (dwc_data)
 			dwc->quirks = dwc_data->quirks;
 
-		if (!(dwc->quirks & DWC3_QUIRK_NO_USB2_PHY))
+		if (!(dwc->quirks & DWC3_QUIRK_NO_USB2_PHY)) {
+			dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
 			dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev,
 				"usb-phy", 0);
+		}
 
-		if (!(dwc->quirks & DWC3_QUIRK_NO_USB3_PHY))
+		if (!(dwc->quirks & DWC3_QUIRK_NO_USB3_PHY)) {
+			dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
 			dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev,
 				"usb-phy", 1);
+		}
 
 		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;
 
-		if (!(pdata->quirks & DWC3_QUIRK_NO_USB2_PHY))
+		if (!(pdata->quirks & DWC3_QUIRK_NO_USB2_PHY)) {
+			dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
 			dwc->usb2_phy = devm_usb_get_phy(dev,
 				USB_PHY_TYPE_USB2);
+		}
 
-		if (!(pdata->quirks & DWC3_QUIRK_NO_USB3_PHY))
+		if (!(pdata->quirks & DWC3_QUIRK_NO_USB3_PHY)) {
+			dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
 			dwc->usb3_phy = devm_usb_get_phy(dev,
 				USB_PHY_TYPE_USB3);
+		}
 
 		dwc->needs_fifo_resize = pdata->tx_fifo_resize;
 		dwc->dr_mode = pdata->dr_mode;
@@ -445,14 +465,20 @@ static int dwc3_probe(struct platform_device *pdev)
 		dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
 	}
 
-	if (IS_ERR(dwc->usb2_phy)) {
+	if (IS_ERR(dwc->usb2_phy) && IS_ERR(dwc->usb2_generic_phy)) {
 		dev_err(dev, "usb2 phy not found\n");
-		return PTR_ERR(dwc->usb2_phy);
+		if (PTR_ERR(dwc->usb2_generic_phy) == -EPROBE_DEFER ||
+			PTR_ERR(dwc->usb2_phy) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		return -ENODEV;
 	}
 
-	if (IS_ERR(dwc->usb3_phy)) {
+	if (IS_ERR(dwc->usb3_phy) && IS_ERR(dwc->usb3_generic_phy)) {
 		dev_err(dev, "usb3 phy not found\n");
-		return PTR_ERR(dwc->usb3_phy);
+		if (PTR_ERR(dwc->usb3_generic_phy) == -EPROBE_DEFER ||
+			PTR_ERR(dwc->usb3_phy) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		return -ENODEV;
 	}
 
 	/* default to superspeed if no maximum_speed passed */
@@ -510,6 +536,11 @@ static int dwc3_probe(struct platform_device *pdev)
 	if (!IS_ERR(dwc->usb3_phy))
 		usb_phy_set_suspend(dwc->usb3_phy, 0);
 
+	if (!IS_ERR(dwc->usb2_generic_phy))
+		phy_power_on(dwc->usb2_generic_phy);
+	if (!IS_ERR(dwc->usb3_generic_phy))
+		phy_power_on(dwc->usb3_generic_phy);
+
 	ret = dwc3_event_buffers_setup(dwc);
 	if (ret) {
 		dev_err(dwc->dev, "failed to setup event buffers\n");
@@ -595,6 +626,10 @@ err1:
 		usb_phy_set_suspend(dwc->usb2_phy, 1);
 	if (!IS_ERR(dwc->usb3_phy))
 		usb_phy_set_suspend(dwc->usb3_phy, 1);
+	if (!IS_ERR(dwc->usb2_generic_phy))
+		phy_power_off(dwc->usb2_generic_phy);
+	if (!IS_ERR(dwc->usb3_generic_phy))
+		phy_power_off(dwc->usb3_generic_phy);
 	dwc3_core_exit(dwc);
 
 err0:
@@ -612,6 +647,11 @@ static int dwc3_remove(struct platform_device *pdev)
 	if (!IS_ERR(dwc->usb3_phy))
 		usb_phy_set_suspend(dwc->usb3_phy, 1);
 
+	if (!IS_ERR(dwc->usb2_generic_phy))
+		phy_power_off(dwc->usb2_generic_phy);
+	if (!IS_ERR(dwc->usb3_generic_phy))
+		phy_power_off(dwc->usb3_generic_phy);
+
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
@@ -711,6 +751,11 @@ static int dwc3_suspend(struct device *dev)
 	if (!IS_ERR(dwc->usb2_phy))
 		usb_phy_shutdown(dwc->usb2_phy);
 
+	if (!IS_ERR(dwc->usb2_generic_phy))
+		phy_exit(dwc->usb2_generic_phy);
+	if (!IS_ERR(dwc->usb3_generic_phy))
+		phy_exit(dwc->usb3_generic_phy);
+
 	return 0;
 }
 
@@ -724,6 +769,11 @@ static int dwc3_resume(struct device *dev)
 	if (!IS_ERR(dwc->usb2_phy))
 		usb_phy_init(dwc->usb2_phy);
 
+	if (!IS_ERR(dwc->usb2_generic_phy))
+		phy_init(dwc->usb2_generic_phy);
+	if (!IS_ERR(dwc->usb3_generic_phy))
+		phy_init(dwc->usb3_generic_phy);
+
 	spin_lock_irqsave(&dwc->lock, flags);
 
 	dwc3_writel(dwc->regs, DWC3_GCTL, dwc->gctl);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 1e11dd1..dd284ab 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
@@ -617,6 +619,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
@@ -670,6 +674,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;
 
-- 
1.7.10.4

[PATCH v3 06/10] 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}    |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
 rename Documentation/devicetree/bindings/{usb/usb-phy.txt => phy/ti-phy.txt} (88%)

diff --git a/Documentation/devicetree/bindings/usb/usb-phy.txt b/Documentation/devicetree/bindings/phy/ti-phy.txt
similarity index 88%
rename from Documentation/devicetree/bindings/usb/usb-phy.txt
rename to Documentation/devicetree/bindings/phy/ti-phy.txt
index c0245c8..207e14c 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,10 @@ 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,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 +38,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 v3 07/10] 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                                |   11 +
 drivers/phy/Makefile                               |    1 +
 .../phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c}     |  232 ++++++++++++--------
 drivers/usb/phy/Kconfig                            |   11 -
 drivers/usb/phy/Makefile                           |    1 -
 5 files changed, 149 insertions(+), 107 deletions(-)
 rename drivers/{usb/phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c} (55%)

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index a344f3d..1abbfcc 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -33,6 +33,17 @@ config OMAP_USB2
 	  The USB OTG controller communicates with the comparator using this
 	  driver.
 
+config TI_PIPE3
+	tristate "TI PIPE3 PHY Driver"
+	depends on ARCH_OMAP2PLUS || COMPILE_TEST
+	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 d0caae9..94a1a79 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -6,4 +6,5 @@ obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
 obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-exynos-dp-video.o
 obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.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 55%
rename from drivers/usb/phy/phy-omap-usb3.c
rename to drivers/phy/phy-ti-pipe3.c
index 0c6ba29..410b286 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.
  *
  * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
  * This program is free software; you can redistribute it and/or modify
@@ -19,10 +19,11 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
-#include <linux/usb/omap_usb.h>
+#include <linux/phy/phy.h>
 #include <linux/of.h>
 #include <linux/clk.h>
 #include <linux/err.h>
+#include <linux/io.h>
 #include <linux/pm_runtime.h>
 #include <linux/delay.h>
 #include <linux/usb/omap_control_usb.h>
@@ -52,17 +53,34 @@
 
 /*
  * 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_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;
+};
+
+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 +89,18 @@ 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 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);
+}
+
+static struct pipe3_dpll_params *ti_pipe3_get_dpll_params(unsigned long rate)
 {
 	int i;
 
@@ -83,110 +112,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 +227,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 +249,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 +260,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 +285,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 +299,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 +329,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 +339,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 +365,39 @@ 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-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 08e2f39..f275751 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 022c1da..220c6df 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
-- 
1.7.10.4

Re: [PATCH v3 07/10] drivers: phy: usb3/pipe3: Adapt pipe3 driver to Generic PHY Framework

[Roger Quadros]

Hi Kishon,

On 11/25/2013 12:01 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                                |   11 +
>  drivers/phy/Makefile                               |    1 +
>  .../phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c}     |  232 ++++++++++++--------
>  drivers/usb/phy/Kconfig                            |   11 -
>  drivers/usb/phy/Makefile                           |    1 -
>  5 files changed, 149 insertions(+), 107 deletions(-)
>  rename drivers/{usb/phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c} (55%)
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index a344f3d..1abbfcc 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -33,6 +33,17 @@ config OMAP_USB2
>  	  The USB OTG controller communicates with the comparator using this
>  	  driver.
>  
> +config TI_PIPE3
> +	tristate "TI PIPE3 PHY Driver"
> +	depends on ARCH_OMAP2PLUS || COMPILE_TEST
> +	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 d0caae9..94a1a79 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -6,4 +6,5 @@ obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
>  obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-exynos-dp-video.o
>  obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.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 55%
> rename from drivers/usb/phy/phy-omap-usb3.c
> rename to drivers/phy/phy-ti-pipe3.c
> index 0c6ba29..410b286 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.
>   *
>   * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
>   * This program is free software; you can redistribute it and/or modify
> @@ -19,10 +19,11 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> -#include <linux/usb/omap_usb.h>
> +#include <linux/phy/phy.h>
>  #include <linux/of.h>
>  #include <linux/clk.h>
>  #include <linux/err.h>
> +#include <linux/io.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/delay.h>
>  #include <linux/usb/omap_control_usb.h>
> @@ -52,17 +53,34 @@
>  
>  /*
>   * 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_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;
> +};
> +
> +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 +89,18 @@ 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 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);
> +}
> +
> +static struct pipe3_dpll_params *ti_pipe3_get_dpll_params(unsigned long rate)
>  {
>  	int i;
>  
> @@ -83,110 +112,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);

I had suggested to use sleep instead of udelay here but usleep for < 10 us might not be not optimal.
see Documentation/timers/timers-howto.txt

Why can't we just use msleep(1)?

Do we know approximately how much time it takes for the block to power down?

> +	} while (--timeout);
> +

what if there was a timeout? you need to exit with return code and preferably print an error message.

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

here as well.
>  
>  	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)));
>  }

cheers,
-roger

Re: [PATCH v3 07/10] drivers: phy: usb3/pipe3: Adapt pipe3 driver to Generic PHY Framework

[Kishon Vijay Abraham I]

Hi,

On Friday 06 December 2013 08:05 PM, Roger Quadros wrote:
> Hi Kishon,
> 
> On 11/25/2013 12:01 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                                |   11 +
>>  drivers/phy/Makefile                               |    1 +
>>  .../phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c}     |  232 ++++++++++++--------
>>  drivers/usb/phy/Kconfig                            |   11 -
>>  drivers/usb/phy/Makefile                           |    1 -
>>  5 files changed, 149 insertions(+), 107 deletions(-)
>>  rename drivers/{usb/phy/phy-omap-usb3.c => phy/phy-ti-pipe3.c} (55%)
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index a344f3d..1abbfcc 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -33,6 +33,17 @@ config OMAP_USB2
>>  	  The USB OTG controller communicates with the comparator using this
>>  	  driver.
>>  
>> +config TI_PIPE3
>> +	tristate "TI PIPE3 PHY Driver"
>> +	depends on ARCH_OMAP2PLUS || COMPILE_TEST
>> +	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 d0caae9..94a1a79 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -6,4 +6,5 @@ obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
>>  obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-exynos-dp-video.o
>>  obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.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 55%
>> rename from drivers/usb/phy/phy-omap-usb3.c
>> rename to drivers/phy/phy-ti-pipe3.c
>> index 0c6ba29..410b286 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.
>>   *
>>   * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
>>   * This program is free software; you can redistribute it and/or modify
>> @@ -19,10 +19,11 @@
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/slab.h>
>> -#include <linux/usb/omap_usb.h>
>> +#include <linux/phy/phy.h>
>>  #include <linux/of.h>
>>  #include <linux/clk.h>
>>  #include <linux/err.h>
>> +#include <linux/io.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/delay.h>
>>  #include <linux/usb/omap_control_usb.h>
>> @@ -52,17 +53,34 @@
>>  
>>  /*
>>   * 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_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;
>> +};
>> +
>> +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 +89,18 @@ 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 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);
>> +}
>> +
>> +static struct pipe3_dpll_params *ti_pipe3_get_dpll_params(unsigned long rate)
>>  {
>>  	int i;
>>  
>> @@ -83,110 +112,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);
> 
> I had suggested to use sleep instead of udelay here but usleep for < 10 us might not be not optimal.
> see Documentation/timers/timers-howto.txt

ok.
> 
> Why can't we just use msleep(1)?

isn't it too long?
> 
> Do we know approximately how much time it takes for the block to power down?

I have to check that. But long time back for OMAP5 it used to vary from board
to board.
> 
>> +	} while (--timeout);
>> +
> 
> what if there was a timeout? you need to exit with return code and preferably print an error message.

hmm.. yeah.
> 
>> +	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);
> 
> here as well.

ok.

Thanks
Kishon

[PATCH v3 08/10] 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>
---
 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 v3 09/10] 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 v3 10/10] 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 |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/omap5.dtsi b/arch/arm/boot/dts/omap5.dtsi
index 29a6354..c135677 100644
--- a/arch/arm/boot/dts/omap5.dtsi
+++ b/arch/arm/boot/dts/omap5.dtsi
@@ -678,6 +678,8 @@
 				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";
 				dr_mode = "peripheral";
 				tx-fifo-resize;
 			};
@@ -694,6 +696,7 @@
 				compatible = "ti,omap-usb2";
 				reg = <0x4a084000 0x7c>;
 				ctrl-module = <&omap_control_usb2phy>;
+				#phy-cells = <0>;
 			};
 
 			usb3_phy: usb3phy@4a084400 {
@@ -703,6 +706,7 @@
 				      <0x4a084c00 0x40>;
 				reg-names = "phy_rx", "phy_tx", "pll_ctrl";
 				ctrl-module = <&omap_control_usb3phy>;
+				#phy-cells = <0>;
 			};
 		};
 
-- 
1.7.10.4

[PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Kishon Vijay Abraham I]

Since PHYs for dwc3 is optional (not all SoCs that have DWC3 use PHYs),
do not return from probe if the USB PHY library returns -ENODEV as that
indicates the platform does not have PHY.

Signed-off-by: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
---
 drivers/usb/dwc3/core.c |   34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index a49217a..e009d4e 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -411,32 +411,26 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	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)
+		if (ret == -ENXIO || ret == -ENODEV) {
+			dwc->usb2_phy = NULL;
+		} else if (ret == -EPROBE_DEFER) {
 			return ret;
-
-		dev_err(dev, "no usb2 phy configured\n");
-		return -EPROBE_DEFER;
+		} else {
+			dev_err(dev, "no usb2 phy configured\n");
+			return ret;
+		}
 	}
 
 	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)
+		if (ret == -ENXIO || ret == -ENODEV) {
+			dwc->usb3_phy = NULL;
+		} else if (ret == -EPROBE_DEFER) {
 			return ret;

[PATCH v4 2/2] 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-l0cyMroinI0@public.gmane.org>
---
Changes from v3:
* avoided using quirks

 Documentation/devicetree/bindings/usb/dwc3.txt |    6 ++-
 drivers/usb/dwc3/core.c                        |   60 ++++++++++++++++++++++++
 drivers/usb/dwc3/core.h                        |    7 +++
 3 files changed, 71 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/core.c b/drivers/usb/dwc3/core.c
index e009d4e..036d589 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -82,6 +82,11 @@ 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 +348,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_exit(dwc->usb2_generic_phy);
+	if (dwc->usb3_generic_phy)
+		phy_exit(dwc->usb3_generic_phy);
+
 }
 
 #define DWC3_ALIGN_MASK		(16 - 1)
@@ -433,6 +443,32 @@ static int dwc3_probe(struct platform_device *pdev)
 		}
 	}
 
+	dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
+	if (IS_ERR(dwc->usb2_generic_phy)) {
+		ret = PTR_ERR(dwc->usb2_generic_phy);
+		if (ret == -ENOSYS || ret == -ENODEV) {
+			dwc->usb2_generic_phy = NULL;
+		} else if (ret == -EPROBE_DEFER) {
+			return ret;
+		} else {
+			dev_err(dev, "no usb2 phy configured\n");
+			return ret;
+		}
+	}
+
+	dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
+	if (IS_ERR(dwc->usb3_generic_phy)) {
+		ret = PTR_ERR(dwc->usb3_generic_phy);
+		if (ret == -ENOSYS || ret == -ENODEV) {
+			dwc->usb3_generic_phy = NULL;
+		} else if (ret == -EPROBE_DEFER) {
+			return ret;
+		} else {
+			dev_err(dev, "no usb3 phy configured\n");
+			return ret;
+		}
+	}
+
 	dwc->xhci_resources[0].start = res->start;
 	dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
 					DWC3_XHCI_REGS_END;
@@ -482,6 +518,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);
+
 	ret = dwc3_event_buffers_setup(dwc);
 	if (ret) {
 		dev_err(dwc->dev, "failed to setup event buffers\n");
@@ -565,6 +606,10 @@ err2:
 err1:
 	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);
 	dwc3_core_exit(dwc);
 
 err0:
@@ -580,6 +625,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_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
@@ -677,6 +727,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;
 }
 
@@ -688,6 +743,11 @@ 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);
+
 	spin_lock_irqsave(&dwc->lock, flags);
 
 	dwc3_writel(dwc->regs, DWC3_GCTL, dwc->gctl);
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;
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Roger Quadros]

Hi Kishon,

On 01/21/2014 12:11 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>
> ---
> Changes from v3:
> * avoided using quirks
> 
>  Documentation/devicetree/bindings/usb/dwc3.txt |    6 ++-
>  drivers/usb/dwc3/core.c                        |   60 ++++++++++++++++++++++++
>  drivers/usb/dwc3/core.h                        |    7 +++
>  3 files changed, 71 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/core.c b/drivers/usb/dwc3/core.c
> index e009d4e..036d589 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -82,6 +82,11 @@ 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);

What if phy_init() fails? You need to report and fail. Same applies for all PHY apis in this patch.

> +	if (dwc->usb3_generic_phy)
> +		phy_init(dwc->usb3_generic_phy);
> +
>  	mdelay(100);
>  
>  	/* Clear USB3 PHY reset */
> @@ -343,6 +348,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_exit(dwc->usb2_generic_phy);
> +	if (dwc->usb3_generic_phy)
> +		phy_exit(dwc->usb3_generic_phy);
> +
>  }
>  
>  #define DWC3_ALIGN_MASK		(16 - 1)
> @@ -433,6 +443,32 @@ static int dwc3_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> +	if (IS_ERR(dwc->usb2_generic_phy)) {
> +		ret = PTR_ERR(dwc->usb2_generic_phy);
> +		if (ret == -ENOSYS || ret == -ENODEV) {
> +			dwc->usb2_generic_phy = NULL;
> +		} else if (ret == -EPROBE_DEFER) {
> +			return ret;
> +		} else {
> +			dev_err(dev, "no usb2 phy configured\n");
> +			return ret;
> +		}
> +	}
> +
> +	dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> +	if (IS_ERR(dwc->usb3_generic_phy)) {
> +		ret = PTR_ERR(dwc->usb3_generic_phy);
> +		if (ret == -ENOSYS || ret == -ENODEV) {
> +			dwc->usb3_generic_phy = NULL;
> +		} else if (ret == -EPROBE_DEFER) {
> +			return ret;
> +		} else {
> +			dev_err(dev, "no usb3 phy configured\n");
> +			return ret;
> +		}
> +	}
> +
>  	dwc->xhci_resources[0].start = res->start;
>  	dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
>  					DWC3_XHCI_REGS_END;
> @@ -482,6 +518,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);
> +

Is it OK to power on the phy before phy_init()?

I suggest to move phy_init() from core_soft_reset() to here, just before phy_power_on().

>  	ret = dwc3_event_buffers_setup(dwc);
>  	if (ret) {
>  		dev_err(dwc->dev, "failed to setup event buffers\n");
> @@ -565,6 +606,10 @@ err2:
>  err1:
>  	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);
>  	dwc3_core_exit(dwc);
>  
>  err0:
> @@ -580,6 +625,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_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
> @@ -677,6 +727,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;
>  }
>  
> @@ -688,6 +743,11 @@ 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);
> +
>  	spin_lock_irqsave(&dwc->lock, flags);
>  
>  	dwc3_writel(dwc->regs, DWC3_GCTL, dwc->gctl);

cheers,
-roger

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

[Vivek Gautam]

Hi,


On Tue, Jan 21, 2014 at 7:30 PM, Roger Quadros <rogerq@ti.com> wrote:
> Hi Kishon,
>
> On 01/21/2014 12:11 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>
>> ---
>> Changes from v3:
>> * avoided using quirks
>>
>>  Documentation/devicetree/bindings/usb/dwc3.txt |    6 ++-
>>  drivers/usb/dwc3/core.c                        |   60 ++++++++++++++++++++++++
>>  drivers/usb/dwc3/core.h                        |    7 +++
>>  3 files changed, 71 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/core.c b/drivers/usb/dwc3/core.c
>> index e009d4e..036d589 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -82,6 +82,11 @@ 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);
>
> What if phy_init() fails? You need to report and fail. Same applies for all PHY apis in this patch.
>
>> +     if (dwc->usb3_generic_phy)
>> +             phy_init(dwc->usb3_generic_phy);
>> +
>>       mdelay(100);
>>
>>       /* Clear USB3 PHY reset */
>> @@ -343,6 +348,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_exit(dwc->usb2_generic_phy);
>> +     if (dwc->usb3_generic_phy)
>> +             phy_exit(dwc->usb3_generic_phy);
>> +
>>  }
>>
>>  #define DWC3_ALIGN_MASK              (16 - 1)
>> @@ -433,6 +443,32 @@ static int dwc3_probe(struct platform_device *pdev)
>>               }
>>       }
>>
>> +     dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
>> +     if (IS_ERR(dwc->usb2_generic_phy)) {
>> +             ret = PTR_ERR(dwc->usb2_generic_phy);
>> +             if (ret == -ENOSYS || ret == -ENODEV) {
>> +                     dwc->usb2_generic_phy = NULL;
>> +             } else if (ret == -EPROBE_DEFER) {
>> +                     return ret;
>> +             } else {
>> +                     dev_err(dev, "no usb2 phy configured\n");
>> +                     return ret;
>> +             }
>> +     }
>> +
>> +     dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>> +     if (IS_ERR(dwc->usb3_generic_phy)) {
>> +             ret = PTR_ERR(dwc->usb3_generic_phy);
>> +             if (ret == -ENOSYS || ret == -ENODEV) {
>> +                     dwc->usb3_generic_phy = NULL;
>> +             } else if (ret == -EPROBE_DEFER) {
>> +                     return ret;
>> +             } else {
>> +                     dev_err(dev, "no usb3 phy configured\n");
>> +                     return ret;
>> +             }
>> +     }
>> +
>>       dwc->xhci_resources[0].start = res->start;
>>       dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
>>                                       DWC3_XHCI_REGS_END;
>> @@ -482,6 +518,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);
>> +
>
> Is it OK to power on the phy before phy_init()?

Isn't phy_init() being done before phy_power_on() in the
core_soft_reset() in this patch ?
Isn't that what you want here ?

>
> I suggest to move phy_init() from core_soft_reset() to here, just before phy_power_on().

core_soft_reset() is called before phy_power_on() itself from
dwc3_core_init(), right ?
will moving the phy_inti() here make nay difference ?

>
>>       ret = dwc3_event_buffers_setup(dwc);
>>       if (ret) {
>>               dev_err(dwc->dev, "failed to setup event buffers\n");
[...]
snip


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

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

[Roger Quadros]

On 01/22/2014 08:04 AM, Vivek Gautam wrote:
> Hi,
> 
> 
> On Tue, Jan 21, 2014 at 7:30 PM, Roger Quadros <rogerq@ti.com> wrote:
>> Hi Kishon,
>>
>> On 01/21/2014 12:11 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>
>>> ---
>>> Changes from v3:
>>> * avoided using quirks
>>>
>>>  Documentation/devicetree/bindings/usb/dwc3.txt |    6 ++-
>>>  drivers/usb/dwc3/core.c                        |   60 ++++++++++++++++++++++++
>>>  drivers/usb/dwc3/core.h                        |    7 +++
>>>  3 files changed, 71 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/core.c b/drivers/usb/dwc3/core.c
>>> index e009d4e..036d589 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -82,6 +82,11 @@ 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);
>>
>> What if phy_init() fails? You need to report and fail. Same applies for all PHY apis in this patch.
>>
>>> +     if (dwc->usb3_generic_phy)
>>> +             phy_init(dwc->usb3_generic_phy);
>>> +
>>>       mdelay(100);
>>>
>>>       /* Clear USB3 PHY reset */
>>> @@ -343,6 +348,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_exit(dwc->usb2_generic_phy);
>>> +     if (dwc->usb3_generic_phy)
>>> +             phy_exit(dwc->usb3_generic_phy);
>>> +
>>>  }
>>>
>>>  #define DWC3_ALIGN_MASK              (16 - 1)
>>> @@ -433,6 +443,32 @@ static int dwc3_probe(struct platform_device *pdev)
>>>               }
>>>       }
>>>
>>> +     dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
>>> +     if (IS_ERR(dwc->usb2_generic_phy)) {
>>> +             ret = PTR_ERR(dwc->usb2_generic_phy);
>>> +             if (ret == -ENOSYS || ret == -ENODEV) {
>>> +                     dwc->usb2_generic_phy = NULL;
>>> +             } else if (ret == -EPROBE_DEFER) {
>>> +                     return ret;
>>> +             } else {
>>> +                     dev_err(dev, "no usb2 phy configured\n");
>>> +                     return ret;
>>> +             }
>>> +     }
>>> +
>>> +     dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>>> +     if (IS_ERR(dwc->usb3_generic_phy)) {
>>> +             ret = PTR_ERR(dwc->usb3_generic_phy);
>>> +             if (ret == -ENOSYS || ret == -ENODEV) {
>>> +                     dwc->usb3_generic_phy = NULL;
>>> +             } else if (ret == -EPROBE_DEFER) {
>>> +                     return ret;
>>> +             } else {
>>> +                     dev_err(dev, "no usb3 phy configured\n");
>>> +                     return ret;
>>> +             }
>>> +     }
>>> +
>>>       dwc->xhci_resources[0].start = res->start;
>>>       dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
>>>                                       DWC3_XHCI_REGS_END;
>>> @@ -482,6 +518,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);
>>> +
>>
>> Is it OK to power on the phy before phy_init()?
> 
> Isn't phy_init() being done before phy_power_on() in the
> core_soft_reset() in this patch ?
> Isn't that what you want here ?
> 
>>
>> I suggest to move phy_init() from core_soft_reset() to here, just before phy_power_on().
> 
> core_soft_reset() is called before phy_power_on() itself from
> dwc3_core_init(), right ?
> will moving the phy_inti() here make nay difference ?

You are right. This part is fine then.

cheers,
-roger

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

[Kishon Vijay Abraham I]

On Wednesday 22 January 2014 01:29 PM, Roger Quadros wrote:
> On 01/22/2014 08:04 AM, Vivek Gautam wrote:
>> Hi,
>>
>>
>> On Tue, Jan 21, 2014 at 7:30 PM, Roger Quadros <rogerq@ti.com> wrote:
>>> Hi Kishon,
>>>
>>> On 01/21/2014 12:11 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>
>>>> ---
>>>> Changes from v3:
>>>> * avoided using quirks
>>>>
>>>>  Documentation/devicetree/bindings/usb/dwc3.txt |    6 ++-
>>>>  drivers/usb/dwc3/core.c                        |   60 ++++++++++++++++++++++++
>>>>  drivers/usb/dwc3/core.h                        |    7 +++
>>>>  3 files changed, 71 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/core.c b/drivers/usb/dwc3/core.c
>>>> index e009d4e..036d589 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -82,6 +82,11 @@ 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);
>>>
>>> What if phy_init() fails? You need to report and fail. Same applies for all PHY apis in this patch.
>>>
>>>> +     if (dwc->usb3_generic_phy)
>>>> +             phy_init(dwc->usb3_generic_phy);
>>>> +
>>>>       mdelay(100);
>>>>
>>>>       /* Clear USB3 PHY reset */
>>>> @@ -343,6 +348,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_exit(dwc->usb2_generic_phy);
>>>> +     if (dwc->usb3_generic_phy)
>>>> +             phy_exit(dwc->usb3_generic_phy);
>>>> +
>>>>  }
>>>>
>>>>  #define DWC3_ALIGN_MASK              (16 - 1)
>>>> @@ -433,6 +443,32 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>               }
>>>>       }
>>>>
>>>> +     dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
>>>> +     if (IS_ERR(dwc->usb2_generic_phy)) {
>>>> +             ret = PTR_ERR(dwc->usb2_generic_phy);
>>>> +             if (ret == -ENOSYS || ret == -ENODEV) {
>>>> +                     dwc->usb2_generic_phy = NULL;
>>>> +             } else if (ret == -EPROBE_DEFER) {
>>>> +                     return ret;
>>>> +             } else {
>>>> +                     dev_err(dev, "no usb2 phy configured\n");
>>>> +                     return ret;
>>>> +             }
>>>> +     }
>>>> +
>>>> +     dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>>>> +     if (IS_ERR(dwc->usb3_generic_phy)) {
>>>> +             ret = PTR_ERR(dwc->usb3_generic_phy);
>>>> +             if (ret == -ENOSYS || ret == -ENODEV) {
>>>> +                     dwc->usb3_generic_phy = NULL;
>>>> +             } else if (ret == -EPROBE_DEFER) {
>>>> +                     return ret;
>>>> +             } else {
>>>> +                     dev_err(dev, "no usb3 phy configured\n");
>>>> +                     return ret;
>>>> +             }
>>>> +     }
>>>> +
>>>>       dwc->xhci_resources[0].start = res->start;
>>>>       dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
>>>>                                       DWC3_XHCI_REGS_END;
>>>> @@ -482,6 +518,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);
>>>> +
>>>
>>> Is it OK to power on the phy before phy_init()?
>>
>> Isn't phy_init() being done before phy_power_on() in the
>> core_soft_reset() in this patch ?
>> Isn't that what you want here ?
>>
>>>
>>> I suggest to move phy_init() from core_soft_reset() to here, just before phy_power_on().
>>
>> core_soft_reset() is called before phy_power_on() itself from
>> dwc3_core_init(), right ?
>> will moving the phy_inti() here make nay difference ?
> 
> You are right. This part is fine then.

Yeah.. it was fixed in one of the earlier patches which got merged.

commit 3088f1085d1c08f31f02db26796555a66cdf7ca1
Author: Kishon Vijay Abraham I <kishon@ti.com>
Date:   Mon Nov 25 15:31:21 2013 +0530

    usb: dwc3: invoke phy_resume after phy_init

    usb_phy_set_suspend(phy, 0) is called before usb_phy_init. Fix it.

    Reported-by: Roger Quadros <rogerq@ti.com>
    Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
    Signed-off-by: Felipe Balbi <balbi@ti.com>

Cheers,
Kishon
> 
> cheers,
> -roger
> 

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Roger Quadros]

On 01/21/2014 12:11 PM, Kishon Vijay Abraham I wrote:
> Since PHYs for dwc3 is optional (not all SoCs that have DWC3 use PHYs),
> do not return from probe if the USB PHY library returns -ENODEV as that
> indicates the platform does not have PHY.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>

Reviewed-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>

cheers,
-roger

> ---
>  drivers/usb/dwc3/core.c |   34 ++++++++++++++--------------------
>  1 file changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index a49217a..e009d4e 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -411,32 +411,26 @@ static int dwc3_probe(struct platform_device *pdev)
>  
>  	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)
> +		if (ret == -ENXIO || ret == -ENODEV) {
> +			dwc->usb2_phy = NULL;
> +		} else if (ret == -EPROBE_DEFER) {
>  			return ret;
> -
> -		dev_err(dev, "no usb2 phy configured\n");
> -		return -EPROBE_DEFER;
> +		} else {
> +			dev_err(dev, "no usb2 phy configured\n");
> +			return ret;
> +		}
>  	}
>  
>  	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)
> +		if (ret == -ENXIO || ret == -ENODEV) {
> +			dwc->usb3_phy = NULL;
> +		} else if (ret == -EPROBE_DEFER) {
>  			return ret;
> -
> -		dev_err(dev, "no usb3 phy configured\n");
> -		return -EPROBE_DEFER;
> +		} else {
> +			dev_err(dev, "no usb3 phy configured\n");
> +			return ret;
> +		}
>  	}
>  
>  	dwc->xhci_resources[0].start = res->start;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Felipe Balbi]

[-- Attachment #1.1: Type: text/plain, Size: 1515 bytes --]

On Tue, Jan 21, 2014 at 03:41:38PM +0530, Kishon Vijay Abraham I wrote:
> Since PHYs for dwc3 is optional (not all SoCs that have DWC3 use PHYs),
> do not return from probe if the USB PHY library returns -ENODEV as that

this isn't correct, they all have PHYs, some of them might not be
controllable.

> indicates the platform does not have PHY.

not really, that indicates the current platform tried to grab a PHY and
the PHY doesn't exist. If there's anybody with a non-controllable PHY
and someone gives me a really good reason for not using the generic
no-op PHY, then we should add a flag and we could:

	if (!likely(dwc->flags & DWC3_USB2PHY_DRIVER_NOT_NEEDED))
		dwc3_grab_phys(dwc);

But I really want to see the argument against using no-op. As far as I
could see, everybody needs a PHY driver one way or another, some
platforms just haven't sent any PHY driver upstream and have their own
hacked up solution to avoid using the PHY layer.

Your commit log really needs quite some extra work. What are you trying
to achieve ? Is this related to AM437x which doesn't have a USB3 PHY due
to the way the HW was wired up ? If so, please mention it. Explain how
AM437x HW was wired up, why it lacks a USB3 PHY and why we should
support it the way you want.

That sort of detail needs to be clear in the commit log, specially
considering the peculiar nature of AM43xx which uses a USB3 IP in
USB2-only mode, that needs to be documented in the commit log :-)

cheers

-- 
balbi

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Kishon Vijay Abraham I]

Hi,

On Tuesday 21 January 2014 08:17 PM, Felipe Balbi wrote:
> On Tue, Jan 21, 2014 at 03:41:38PM +0530, Kishon Vijay Abraham I wrote:
>> Since PHYs for dwc3 is optional (not all SoCs that have DWC3 use PHYs),
>> do not return from probe if the USB PHY library returns -ENODEV as that
> 
> this isn't correct, they all have PHYs, some of them might not be
> controllable.

right, but we use USB PHY library only for controllable PHYs (apart from using
nop).
> 
>> indicates the platform does not have PHY.
> 
> not really, that indicates the current platform tried to grab a PHY and
> the PHY doesn't exist. If there's anybody with a non-controllable PHY
> and someone gives me a really good reason for not using the generic
> no-op PHY, then we should add a flag and we could:
> 
> 	if (!likely(dwc->flags & DWC3_USB2PHY_DRIVER_NOT_NEEDED))
> 		dwc3_grab_phys(dwc);
> 
> But I really want to see the argument against using no-op. As far as I
> could see, everybody needs a PHY driver one way or another, some
> platforms just haven't sent any PHY driver upstream and have their own
> hacked up solution to avoid using the PHY layer.

I was trying to address Heikki concerns in my previous version [1] where I used
quirks to identify if the platform does not have PHY.

[1] -> https://lkml.org/lkml/2013/12/5/32

Thanks
Kishon

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Heikki Krogerus]

Hi Felipe,

On Tue, Jan 21, 2014 at 08:47:25AM -0600, Felipe Balbi wrote:
> On Tue, Jan 21, 2014 at 03:41:38PM +0530, Kishon Vijay Abraham I wrote:
> > Since PHYs for dwc3 is optional (not all SoCs that have DWC3 use PHYs),
> > do not return from probe if the USB PHY library returns -ENODEV as that
> 
> this isn't correct, they all have PHYs, some of them might not be
> controllable.
> 
> > indicates the platform does not have PHY.
> 
> not really, that indicates the current platform tried to grab a PHY and
> the PHY doesn't exist. If there's anybody with a non-controllable PHY
> and someone gives me a really good reason for not using the generic
> no-op PHY, then we should add a flag and we could:
> 
> 	if (!likely(dwc->flags & DWC3_USB2PHY_DRIVER_NOT_NEEDED))
> 		dwc3_grab_phys(dwc);

Why would you need to know if the PHY drivers are needed or not
explicitly in your controller driver?

> But I really want to see the argument against using no-op. As far as I
> could see, everybody needs a PHY driver one way or another, some
> platforms just haven't sent any PHY driver upstream and have their own
> hacked up solution to avoid using the PHY layer.

Not true in our case. Platforms using Intel's SoCs and chip sets may
or may not have controllable USB PHY. Quite often they don't. The
Baytrails have usually ULPI PHY for USB2, but that does not mean they
provide any vendor specific functions or any need for a driver in any
case.

Are we talking about the old USB PHY library or the new PHY framework
with the no-op PHY driver?

Well, in any case, I don't understand what is the purpose of the no-op
PHY driver. What are you drying to achieve with that?


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Felipe Balbi]

[-- Attachment #1.1: Type: text/plain, Size: 2823 bytes --]

Hi,

On Mon, Jan 27, 2014 at 05:08:55PM +0200, Heikki Krogerus wrote:
> > > Since PHYs for dwc3 is optional (not all SoCs that have DWC3 use PHYs),
> > > do not return from probe if the USB PHY library returns -ENODEV as that
> > 
> > this isn't correct, they all have PHYs, some of them might not be
> > controllable.
> > 
> > > indicates the platform does not have PHY.
> > 
> > not really, that indicates the current platform tried to grab a PHY and
> > the PHY doesn't exist. If there's anybody with a non-controllable PHY
> > and someone gives me a really good reason for not using the generic
> > no-op PHY, then we should add a flag and we could:
> > 
> > 	if (!likely(dwc->flags & DWC3_USB2PHY_DRIVER_NOT_NEEDED))
> > 		dwc3_grab_phys(dwc);
> 
> Why would you need to know if the PHY drivers are needed or not
> explicitly in your controller driver?

because, one way or another, they all do need it. Except for quirky ones
like AM437x where a USB3 IP was hardwired into USB2-only mode, so it
really lacks a USB3 PHY.

> > But I really want to see the argument against using no-op. As far as I
> > could see, everybody needs a PHY driver one way or another, some
> > platforms just haven't sent any PHY driver upstream and have their own
> > hacked up solution to avoid using the PHY layer.
> 
> Not true in our case. Platforms using Intel's SoCs and chip sets may
> or may not have controllable USB PHY. Quite often they don't. The
> Baytrails have usually ULPI PHY for USB2, but that does not mean they
> provide any vendor specific functions or any need for a driver in any
> case.

that's different from what I heard.

> Are we talking about the old USB PHY library or the new PHY framework
> with the no-op PHY driver?
> 
> Well, in any case, I don't understand what is the purpose of the no-op
> PHY driver. What are you drying to achieve with that?

I'm trying to avoid supporting 500 different combinations for a single
driver. We already support old USB PHY layer and generic PHY layer, now
they both need to be made optional. The old USB PHY layer also provides
a no-op, now called phy-generic, which is actually pretty useful.

On top of all that, I'm sure you'll subscribe to the idea of preventing
dwc3 from becoming another MUSB which supports several different
configurations and none work correctly. I much prefer to take baby steps
which are well thought-out and very well exercised, so I'll be very
pedantic about proof of testing.

An invasive change such as $subject needs to be very well-tested in
different architectures with different Kconfig choices before I'd feel
comfortable applying it.

Also, the assumptions made in $subject are just plain wrong, which
gets me really iffy about applying it or not.

cheers

-- 
balbi

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Heikki Krogerus]

Hi,

On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
> > Why would you need to know if the PHY drivers are needed or not
> > explicitly in your controller driver?
> 
> because, one way or another, they all do need it. Except for quirky ones
> like AM437x where a USB3 IP was hardwired into USB2-only mode, so it
> really lacks a USB3 PHY.

The Baytrail board I deal with has completely autonomous PHYs. But my
question is, why would you need to care about the PHYs in your
controller driver? Why can't you leave the responsibility of them to
the upper layers and trust what they tell you?

If there is no USB3 PHY for dwc3 then, the driver gets something like
-ENODEV and just continues. There is no need to know about the
details.

For the controller drivers the PHYs are just a resource like any
other. The controller drivers can't have any responsibility of
them. They should not care if PHY drivers are available for them or
not, or even if the PHY framework is available or not.

> > > But I really want to see the argument against using no-op. As far as I
> > > could see, everybody needs a PHY driver one way or another, some
> > > platforms just haven't sent any PHY driver upstream and have their own
> > > hacked up solution to avoid using the PHY layer.
> > 
> > Not true in our case. Platforms using Intel's SoCs and chip sets may
> > or may not have controllable USB PHY. Quite often they don't. The
> > Baytrails have usually ULPI PHY for USB2, but that does not mean they
> > provide any vendor specific functions or any need for a driver in any
> > case.
> 
> that's different from what I heard.

I don't know where you got that impression, but it's not true. The
Baytrail SoCs for example don't have internal USB PHYs, which means
the manufacturers using it can select what they want. So we have
boards where PHY driver(s) is needed and boards where it isn't.

The problem is that we just don't always know all the details about
the platform. If the PHY is ULPI PHY, we can do runtime detection, but
we can't even rely on always having ULPI.

> > Are we talking about the old USB PHY library or the new PHY framework
> > with the no-op PHY driver?
> > 
> > Well, in any case, I don't understand what is the purpose of the no-op
> > PHY driver. What are you drying to achieve with that?
> 
> I'm trying to avoid supporting 500 different combinations for a single
> driver. We already support old USB PHY layer and generic PHY layer, now
> they both need to be made optional.

This is really good to get. We have some projects where we are dealing
with more embedded environments, like IVI, where the kernel should be
stripped of everything useless. Since the PHYs are autonomous, we
should be able to disable the PHY libraries/frameworks.

But I still don't understand what is the benefit in the no-op? You
really need to explain this. What you have now in dwc3 is expectations
regarding the PHY, which put a lot of pressure on upper layers to
satisfy the driver. The no-op is just an extra thing that you have to
provide when you fail to determine if the system requires a PHY driver
or not, or if you know that it doesn't, plus an additional check for
the case where the PHY lib/framework is not enabled. I don't see the
value in it.

> The old USB PHY layer also provides
> a no-op, now called phy-generic, which is actually pretty useful.
> 
> On top of all that, I'm sure you'll subscribe to the idea of preventing
> dwc3 from becoming another MUSB which supports several different
> configurations and none work correctly. I much prefer to take baby steps
> which are well thought-out and very well exercised, so I'll be very
> pedantic about proof of testing.

I think our goals are the same. I just want to also minimize the need
for any platform specific extra work from the upper layers regarding
the PHYs.


Thanks,

-- 
heikki

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Felipe Balbi]

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

Hi,

On Tue, Jan 28, 2014 at 05:32:30PM +0200, Heikki Krogerus wrote:
> On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
> > > Why would you need to know if the PHY drivers are needed or not
> > > explicitly in your controller driver?
> > 
> > because, one way or another, they all do need it. Except for quirky ones
> > like AM437x where a USB3 IP was hardwired into USB2-only mode, so it
> > really lacks a USB3 PHY.
> 
> The Baytrail board I deal with has completely autonomous PHYs. But my
> question is, why would you need to care about the PHYs in your
> controller driver? Why can't you leave the responsibility of them to
> the upper layers and trust what they tell you?
> 
> If there is no USB3 PHY for dwc3 then, the driver gets something like
> -ENODEV and just continues. There is no need to know about the
> details.

it's a little more complicated than that. We could get -EPROBE_DEFER
meaning we should try probing at a later time because the PHY isn't
available yet.

But sure, if we can very easily differentiate between those two
conditions in a way that error report from PHY layer (whichever it is),
then we can certainly do that.

> For the controller drivers the PHYs are just a resource like any
> other. The controller drivers can't have any responsibility of
> them. They should not care if PHY drivers are available for them or
> not, or even if the PHY framework is available or not.

huh? If memory isn't available you don't continue probing, right ? If
your IORESOURCE_MEM is missing, you also don't continue probing, if your
IRQ line is missing, you bail too. Those are also nothing but resources
to the driver, what you're asking here is to treat PHY as a _different_
resource; which might be fine, but we need to make sure we don't
continue probing when a PHY is missing in a platform that certainly
needs a PHY.

> > > > But I really want to see the argument against using no-op. As far as I
> > > > could see, everybody needs a PHY driver one way or another, some
> > > > platforms just haven't sent any PHY driver upstream and have their own
> > > > hacked up solution to avoid using the PHY layer.
> > > 
> > > Not true in our case. Platforms using Intel's SoCs and chip sets may
> > > or may not have controllable USB PHY. Quite often they don't. The
> > > Baytrails have usually ULPI PHY for USB2, but that does not mean they
> > > provide any vendor specific functions or any need for a driver in any
> > > case.
> > 
> > that's different from what I heard.
> 
> I don't know where you got that impression, but it's not true. The
> Baytrail SoCs for example don't have internal USB PHYs, which means
> the manufacturers using it can select what they want. So we have
> boards where PHY driver(s) is needed and boards where it isn't.

alright, that explains it ;-) So you have external USB2 and USB3 PHYs ?
You have an external PIPE3 interface ? That's quite an achievement,
kudos to your HW designers. Getting timing closure on PIPE3 is a
difficult task.

> The problem is that we just don't always know all the details about
> the platform. If the PHY is ULPI PHY, we can do runtime detection, but
> we can't even rely on always having ULPI.

right, we've had that before on n900 ;-)

> > > Are we talking about the old USB PHY library or the new PHY framework
> > > with the no-op PHY driver?
> > > 
> > > Well, in any case, I don't understand what is the purpose of the no-op
> > > PHY driver. What are you drying to achieve with that?
> > 
> > I'm trying to avoid supporting 500 different combinations for a single
> > driver. We already support old USB PHY layer and generic PHY layer, now
> > they both need to be made optional.
> 
> This is really good to get. We have some projects where we are dealing
> with more embedded environments, like IVI, where the kernel should be
> stripped of everything useless. Since the PHYs are autonomous, we
> should be able to disable the PHY libraries/frameworks.

hmmm, in that case it's a lot easier to treat. We can use
ERR_PTR(-ENXIO) as an indication that the framework is disabled, or
something like that.

The difficult is really reliably supporting e.g. OMAP5 (which won't work
without a PHY) and your BayTrail with autonomous PHYs. What can we use
as an indication ?

I mean, I need to know that a particular platform depends on a PHY
driver before I decide to return -EPROBE_DEFER or just assume the PHY
isn't needed ;-)

> But I still don't understand what is the benefit in the no-op? You
> really need to explain this. What you have now in dwc3 is expectations
> regarding the PHY, which put a lot of pressure on upper layers to
> satisfy the driver. The no-op is just an extra thing that you have to
> provide when you fail to determine if the system requires a PHY driver
> or not, or if you know that it doesn't, plus an additional check for
> the case where the PHY lib/framework is not enabled. I don't see the
> value in it.

it "solves" the difficulty above. If everybody has to provide a PHY
driver, the problem is a lot simpler don't you think ? ;-)

> > The old USB PHY layer also provides
> > a no-op, now called phy-generic, which is actually pretty useful.
> > 
> > On top of all that, I'm sure you'll subscribe to the idea of preventing
> > dwc3 from becoming another MUSB which supports several different
> > configurations and none work correctly. I much prefer to take baby steps
> > which are well thought-out and very well exercised, so I'll be very
> > pedantic about proof of testing.
> 
> I think our goals are the same. I just want to also minimize the need
> for any platform specific extra work from the upper layers regarding
> the PHYs.

I'll agree to that, but let's not apply patches until we're 100% sure
there will be no regressions. Looking at $subject, I don't think it
satisfies that condition ;-)

-- 
balbi

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

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Heikki Krogerus]

Hi,

On Tue, Jan 28, 2014 at 10:30:36AM -0600, Felipe Balbi wrote:
> On Tue, Jan 28, 2014 at 05:32:30PM +0200, Heikki Krogerus wrote:
> > On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
> > For the controller drivers the PHYs are just a resource like any
> > other. The controller drivers can't have any responsibility of
> > them. They should not care if PHY drivers are available for them or
> > not, or even if the PHY framework is available or not.
> 
> huh? If memory isn't available you don't continue probing, right ? If
> your IORESOURCE_MEM is missing, you also don't continue probing, if your
> IRQ line is missing, you bail too. Those are also nothing but resources
> to the driver, what you're asking here is to treat PHY as a _different_
> resource; which might be fine, but we need to make sure we don't
> continue probing when a PHY is missing in a platform that certainly
> needs a PHY.

Yes, true. In my head I was comparing the PHY only to resources like
gpios, clocks, dma channels, etc. that are often optional to the
drivers.

> > > > > But I really want to see the argument against using no-op. As far as I
> > > > > could see, everybody needs a PHY driver one way or another, some
> > > > > platforms just haven't sent any PHY driver upstream and have their own
> > > > > hacked up solution to avoid using the PHY layer.
> > > > 
> > > > Not true in our case. Platforms using Intel's SoCs and chip sets may
> > > > or may not have controllable USB PHY. Quite often they don't. The
> > > > Baytrails have usually ULPI PHY for USB2, but that does not mean they
> > > > provide any vendor specific functions or any need for a driver in any
> > > > case.
> > > 
> > > that's different from what I heard.
> > 
> > I don't know where you got that impression, but it's not true. The
> > Baytrail SoCs for example don't have internal USB PHYs, which means
> > the manufacturers using it can select what they want. So we have
> > boards where PHY driver(s) is needed and boards where it isn't.
> 
> alright, that explains it ;-) So you have external USB2 and USB3 PHYs ?
> You have an external PIPE3 interface ? That's quite an achievement,
> kudos to your HW designers. Getting timing closure on PIPE3 is a
> difficult task.

No, only the USB2 PHY is external. I'm giving you wrong information,
I'm sorry about that. Need to concentrate on what I'm writing.

<snip>

> > This is really good to get. We have some projects where we are dealing
> > with more embedded environments, like IVI, where the kernel should be
> > stripped of everything useless. Since the PHYs are autonomous, we
> > should be able to disable the PHY libraries/frameworks.
> 
> hmmm, in that case it's a lot easier to treat. We can use
> ERR_PTR(-ENXIO) as an indication that the framework is disabled, or
> something like that.
> 
> The difficult is really reliably supporting e.g. OMAP5 (which won't work
> without a PHY) and your BayTrail with autonomous PHYs. What can we use
> as an indication ?

OMAP has it's own glue driver, so shouldn't it depend on the PHY
layer?

> I mean, I need to know that a particular platform depends on a PHY
> driver before I decide to return -EPROBE_DEFER or just assume the PHY
> isn't needed ;-)

I don't think dwc3 (core) should care about that. The PHY layer needs
to tell us that. If the PHY driver that the platform depends is not
available yet, the PHY layer returns -EPROBE_DEFER and dwc3 ends up
returning -EPROBE_DEFER.

<snip>

> > I think our goals are the same. I just want to also minimize the need
> > for any platform specific extra work from the upper layers regarding
> > the PHYs.
> 
> I'll agree to that, but let's not apply patches until we're 100% sure
> there will be no regressions. Looking at $subject, I don't think it
> satisfies that condition ;-)

Agreed. Let's think this through carefully.


Cheers,

-- 
heikki

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Kishon Vijay Abraham I]

On Wednesday 29 January 2014 08:17 PM, Heikki Krogerus wrote:
> Hi,
> 
> On Tue, Jan 28, 2014 at 10:30:36AM -0600, Felipe Balbi wrote:
>> On Tue, Jan 28, 2014 at 05:32:30PM +0200, Heikki Krogerus wrote:
>>> On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
>>> For the controller drivers the PHYs are just a resource like any
>>> other. The controller drivers can't have any responsibility of
>>> them. They should not care if PHY drivers are available for them or
>>> not, or even if the PHY framework is available or not.
>>
>> huh? If memory isn't available you don't continue probing, right ? If
>> your IORESOURCE_MEM is missing, you also don't continue probing, if your
>> IRQ line is missing, you bail too. Those are also nothing but resources
>> to the driver, what you're asking here is to treat PHY as a _different_
>> resource; which might be fine, but we need to make sure we don't
>> continue probing when a PHY is missing in a platform that certainly
>> needs a PHY.
> 
> Yes, true. In my head I was comparing the PHY only to resources like
> gpios, clocks, dma channels, etc. that are often optional to the
> drivers.
> 
>>>>>> But I really want to see the argument against using no-op. As far as I
>>>>>> could see, everybody needs a PHY driver one way or another, some
>>>>>> platforms just haven't sent any PHY driver upstream and have their own
>>>>>> hacked up solution to avoid using the PHY layer.
>>>>>
>>>>> Not true in our case. Platforms using Intel's SoCs and chip sets may
>>>>> or may not have controllable USB PHY. Quite often they don't. The
>>>>> Baytrails have usually ULPI PHY for USB2, but that does not mean they
>>>>> provide any vendor specific functions or any need for a driver in any
>>>>> case.
>>>>
>>>> that's different from what I heard.
>>>
>>> I don't know where you got that impression, but it's not true. The
>>> Baytrail SoCs for example don't have internal USB PHYs, which means
>>> the manufacturers using it can select what they want. So we have
>>> boards where PHY driver(s) is needed and boards where it isn't.
>>
>> alright, that explains it ;-) So you have external USB2 and USB3 PHYs ?
>> You have an external PIPE3 interface ? That's quite an achievement,
>> kudos to your HW designers. Getting timing closure on PIPE3 is a
>> difficult task.
> 
> No, only the USB2 PHY is external. I'm giving you wrong information,
> I'm sorry about that. Need to concentrate on what I'm writing.
> 
> <snip>
> 
>>> This is really good to get. We have some projects where we are dealing
>>> with more embedded environments, like IVI, where the kernel should be
>>> stripped of everything useless. Since the PHYs are autonomous, we
>>> should be able to disable the PHY libraries/frameworks.
>>
>> hmmm, in that case it's a lot easier to treat. We can use
>> ERR_PTR(-ENXIO) as an indication that the framework is disabled, or
>> something like that.
>>
>> The difficult is really reliably supporting e.g. OMAP5 (which won't work
>> without a PHY) and your BayTrail with autonomous PHYs. What can we use
>> as an indication ?
> 
> OMAP has it's own glue driver, so shouldn't it depend on the PHY
> layer?

right, but the PHY is connected to the dwc3 core and not to the glue.
> 
>> I mean, I need to know that a particular platform depends on a PHY
>> driver before I decide to return -EPROBE_DEFER or just assume the PHY
>> isn't needed ;-)
> 
> I don't think dwc3 (core) should care about that. The PHY layer needs
> to tell us that. If the PHY driver that the platform depends is not
> available yet, the PHY layer returns -EPROBE_DEFER and dwc3 ends up
> returning -EPROBE_DEFER.

I don't think the PHY layer can 'reliably' tell if PHY driver is available or
not. Consider when the phy_provider_register fails, there is no way to know if
PHY driver is available or not. There are a few cases where PHY layer returns
-EPROBE_DEFER but none of them can tell for sure that PHY driver is either
available and failed or not available at all. It would be best for us to leave
that to the platforms if we want to be sure if the platform needs a PHY or not.

Thanks
Kishon

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Roger Quadros]

Hi,

On 02/12/2014 11:46 AM, Kishon Vijay Abraham I wrote:
> On Wednesday 29 January 2014 08:17 PM, Heikki Krogerus wrote:
>> Hi,
>>
>> On Tue, Jan 28, 2014 at 10:30:36AM -0600, Felipe Balbi wrote:
>>> On Tue, Jan 28, 2014 at 05:32:30PM +0200, Heikki Krogerus wrote:
>>>> On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
>>>> For the controller drivers the PHYs are just a resource like any
>>>> other. The controller drivers can't have any responsibility of
>>>> them. They should not care if PHY drivers are available for them or
>>>> not, or even if the PHY framework is available or not.
>>>
>>> huh? If memory isn't available you don't continue probing, right ? If
>>> your IORESOURCE_MEM is missing, you also don't continue probing, if your
>>> IRQ line is missing, you bail too. Those are also nothing but resources
>>> to the driver, what you're asking here is to treat PHY as a _different_
>>> resource; which might be fine, but we need to make sure we don't
>>> continue probing when a PHY is missing in a platform that certainly
>>> needs a PHY.
>>
>> Yes, true. In my head I was comparing the PHY only to resources like
>> gpios, clocks, dma channels, etc. that are often optional to the
>> drivers.
>>
>>>>>>> But I really want to see the argument against using no-op. As far as I
>>>>>>> could see, everybody needs a PHY driver one way or another, some
>>>>>>> platforms just haven't sent any PHY driver upstream and have their own
>>>>>>> hacked up solution to avoid using the PHY layer.
>>>>>>
>>>>>> Not true in our case. Platforms using Intel's SoCs and chip sets may
>>>>>> or may not have controllable USB PHY. Quite often they don't. The
>>>>>> Baytrails have usually ULPI PHY for USB2, but that does not mean they
>>>>>> provide any vendor specific functions or any need for a driver in any
>>>>>> case.
>>>>>
>>>>> that's different from what I heard.
>>>>
>>>> I don't know where you got that impression, but it's not true. The
>>>> Baytrail SoCs for example don't have internal USB PHYs, which means
>>>> the manufacturers using it can select what they want. So we have
>>>> boards where PHY driver(s) is needed and boards where it isn't.
>>>
>>> alright, that explains it ;-) So you have external USB2 and USB3 PHYs ?
>>> You have an external PIPE3 interface ? That's quite an achievement,
>>> kudos to your HW designers. Getting timing closure on PIPE3 is a
>>> difficult task.
>>
>> No, only the USB2 PHY is external. I'm giving you wrong information,
>> I'm sorry about that. Need to concentrate on what I'm writing.
>>
>> <snip>
>>
>>>> This is really good to get. We have some projects where we are dealing
>>>> with more embedded environments, like IVI, where the kernel should be
>>>> stripped of everything useless. Since the PHYs are autonomous, we
>>>> should be able to disable the PHY libraries/frameworks.
>>>
>>> hmmm, in that case it's a lot easier to treat. We can use
>>> ERR_PTR(-ENXIO) as an indication that the framework is disabled, or
>>> something like that.
>>>
>>> The difficult is really reliably supporting e.g. OMAP5 (which won't work
>>> without a PHY) and your BayTrail with autonomous PHYs. What can we use
>>> as an indication ?
>>
>> OMAP has it's own glue driver, so shouldn't it depend on the PHY
>> layer?
> 
> right, but the PHY is connected to the dwc3 core and not to the glue.
>>
>>> I mean, I need to know that a particular platform depends on a PHY
>>> driver before I decide to return -EPROBE_DEFER or just assume the PHY
>>> isn't needed ;-)
>>
>> I don't think dwc3 (core) should care about that. The PHY layer needs
>> to tell us that. If the PHY driver that the platform depends is not
>> available yet, the PHY layer returns -EPROBE_DEFER and dwc3 ends up
>> returning -EPROBE_DEFER.
> 
> I don't think the PHY layer can 'reliably' tell if PHY driver is available or
> not. Consider when the phy_provider_register fails, there is no way to know if
> PHY driver is available or not. There are a few cases where PHY layer returns
> -EPROBE_DEFER but none of them can tell for sure that PHY driver is either
> available and failed or not available at all. It would be best for us to leave
> that to the platforms if we want to be sure if the platform needs a PHY or not.
> 

Just to summarize this thread on what we need

1) dwc3 core shouldn't worry about platform specific stuff i.e. PHY needed or not.
It should be as generic as possible.

2) dwc3 core should continue probe even if PHY layer is not enabled, as not all platforms need it.

3) dwc3 core should continue probe if PHY device is not available. (-ENODEV?)

4) dwc3 core should error out on any error condition if PHY device is available and caused some error,
e.g. init error.

5) dwc3 core should return EPROBE_DEFER if PHY device is available but device driver is not yet loaded.

6) platform glue should do the necessary sanity checks for availability of all resources like PHY device, PHY layer, etc, before populating the dwc3 device. e.g. in OMAP5 case we could check if both usb2 and usb3 PHY
nodes are available in the DT and PHY layer is enabled, from dwc3-omap.c? In J6 case we could check that at least usb2 phy node is there for the High-Speed only controller, and so on.

Let us first agree on the requirements and then we can think about implementation.

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Kishon Vijay Abraham I]

Hi Roger,

On Wednesday 19 February 2014 06:07 PM, Roger Quadros wrote:
> Hi,
> 
> On 02/12/2014 11:46 AM, Kishon Vijay Abraham I wrote:
>> On Wednesday 29 January 2014 08:17 PM, Heikki Krogerus wrote:
>>> Hi,
>>>
>>> On Tue, Jan 28, 2014 at 10:30:36AM -0600, Felipe Balbi wrote:
>>>> On Tue, Jan 28, 2014 at 05:32:30PM +0200, Heikki Krogerus wrote:
>>>>> On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
>>>>> For the controller drivers the PHYs are just a resource like any
>>>>> other. The controller drivers can't have any responsibility of
>>>>> them. They should not care if PHY drivers are available for them or
>>>>> not, or even if the PHY framework is available or not.
>>>>
>>>> huh? If memory isn't available you don't continue probing, right ? If
>>>> your IORESOURCE_MEM is missing, you also don't continue probing, if your
>>>> IRQ line is missing, you bail too. Those are also nothing but resources
>>>> to the driver, what you're asking here is to treat PHY as a _different_
>>>> resource; which might be fine, but we need to make sure we don't
>>>> continue probing when a PHY is missing in a platform that certainly
>>>> needs a PHY.
>>>
>>> Yes, true. In my head I was comparing the PHY only to resources like
>>> gpios, clocks, dma channels, etc. that are often optional to the
>>> drivers.
>>>
>>>>>>>> But I really want to see the argument against using no-op. As far as I
>>>>>>>> could see, everybody needs a PHY driver one way or another, some
>>>>>>>> platforms just haven't sent any PHY driver upstream and have their own
>>>>>>>> hacked up solution to avoid using the PHY layer.
>>>>>>>
>>>>>>> Not true in our case. Platforms using Intel's SoCs and chip sets may
>>>>>>> or may not have controllable USB PHY. Quite often they don't. The
>>>>>>> Baytrails have usually ULPI PHY for USB2, but that does not mean they
>>>>>>> provide any vendor specific functions or any need for a driver in any
>>>>>>> case.
>>>>>>
>>>>>> that's different from what I heard.
>>>>>
>>>>> I don't know where you got that impression, but it's not true. The
>>>>> Baytrail SoCs for example don't have internal USB PHYs, which means
>>>>> the manufacturers using it can select what they want. So we have
>>>>> boards where PHY driver(s) is needed and boards where it isn't.
>>>>
>>>> alright, that explains it ;-) So you have external USB2 and USB3 PHYs ?
>>>> You have an external PIPE3 interface ? That's quite an achievement,
>>>> kudos to your HW designers. Getting timing closure on PIPE3 is a
>>>> difficult task.
>>>
>>> No, only the USB2 PHY is external. I'm giving you wrong information,
>>> I'm sorry about that. Need to concentrate on what I'm writing.
>>>
>>> <snip>
>>>
>>>>> This is really good to get. We have some projects where we are dealing
>>>>> with more embedded environments, like IVI, where the kernel should be
>>>>> stripped of everything useless. Since the PHYs are autonomous, we
>>>>> should be able to disable the PHY libraries/frameworks.
>>>>
>>>> hmmm, in that case it's a lot easier to treat. We can use
>>>> ERR_PTR(-ENXIO) as an indication that the framework is disabled, or
>>>> something like that.
>>>>
>>>> The difficult is really reliably supporting e.g. OMAP5 (which won't work
>>>> without a PHY) and your BayTrail with autonomous PHYs. What can we use
>>>> as an indication ?
>>>
>>> OMAP has it's own glue driver, so shouldn't it depend on the PHY
>>> layer?
>>
>> right, but the PHY is connected to the dwc3 core and not to the glue.
>>>
>>>> I mean, I need to know that a particular platform depends on a PHY
>>>> driver before I decide to return -EPROBE_DEFER or just assume the PHY
>>>> isn't needed ;-)
>>>
>>> I don't think dwc3 (core) should care about that. The PHY layer needs
>>> to tell us that. If the PHY driver that the platform depends is not
>>> available yet, the PHY layer returns -EPROBE_DEFER and dwc3 ends up
>>> returning -EPROBE_DEFER.
>>
>> I don't think the PHY layer can 'reliably' tell if PHY driver is available or
>> not. Consider when the phy_provider_register fails, there is no way to know if
>> PHY driver is available or not. There are a few cases where PHY layer returns
>> -EPROBE_DEFER but none of them can tell for sure that PHY driver is either
>> available and failed or not available at all. It would be best for us to leave
>> that to the platforms if we want to be sure if the platform needs a PHY or not.
>>
> 
> Just to summarize this thread on what we need

Thanks for summarizing.
> 
> 1) dwc3 core shouldn't worry about platform specific stuff i.e. PHY needed or not.
> It should be as generic as possible.
> 
> 2) dwc3 core should continue probe even if PHY layer is not enabled, as not all platforms need it.
> 
> 3) dwc3 core should continue probe if PHY device is not available. (-ENODEV?)
> 
> 4) dwc3 core should error out on any error condition if PHY device is available and caused some error,
> e.g. init error.
> 
> 5) dwc3 core should return EPROBE_DEFER if PHY device is available but device driver is not yet loaded.
> 
> 6) platform glue should do the necessary sanity checks for availability of all resources like PHY device, PHY layer, etc, before populating the dwc3 device. e.g. in OMAP5 case we could check if both usb2 and usb3 PHY
> nodes are available in the DT and PHY layer is enabled, from dwc3-omap.c? In J6 case we could check that at least usb2 phy node is there for the High-Speed only controller, and so on.

The PHY is connected to the dwc3 core. So I'm not sure if we should be doing
checks for PHY in the glue layer.

Thanks
Kishon

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Roger Quadros]

On 02/21/2014 02:25 PM, Kishon Vijay Abraham I wrote:
> Hi Roger,
> 
> On Wednesday 19 February 2014 06:07 PM, Roger Quadros wrote:
>> Hi,
>>
>> On 02/12/2014 11:46 AM, Kishon Vijay Abraham I wrote:
>>> On Wednesday 29 January 2014 08:17 PM, Heikki Krogerus wrote:
>>>> Hi,
>>>>
>>>> On Tue, Jan 28, 2014 at 10:30:36AM -0600, Felipe Balbi wrote:
>>>>> On Tue, Jan 28, 2014 at 05:32:30PM +0200, Heikki Krogerus wrote:
>>>>>> On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
>>>>>> For the controller drivers the PHYs are just a resource like any
>>>>>> other. The controller drivers can't have any responsibility of
>>>>>> them. They should not care if PHY drivers are available for them or
>>>>>> not, or even if the PHY framework is available or not.
>>>>>
>>>>> huh? If memory isn't available you don't continue probing, right ? If
>>>>> your IORESOURCE_MEM is missing, you also don't continue probing, if your
>>>>> IRQ line is missing, you bail too. Those are also nothing but resources
>>>>> to the driver, what you're asking here is to treat PHY as a _different_
>>>>> resource; which might be fine, but we need to make sure we don't
>>>>> continue probing when a PHY is missing in a platform that certainly
>>>>> needs a PHY.
>>>>
>>>> Yes, true. In my head I was comparing the PHY only to resources like
>>>> gpios, clocks, dma channels, etc. that are often optional to the
>>>> drivers.
>>>>
>>>>>>>>> But I really want to see the argument against using no-op. As far as I
>>>>>>>>> could see, everybody needs a PHY driver one way or another, some
>>>>>>>>> platforms just haven't sent any PHY driver upstream and have their own
>>>>>>>>> hacked up solution to avoid using the PHY layer.
>>>>>>>>
>>>>>>>> Not true in our case. Platforms using Intel's SoCs and chip sets may
>>>>>>>> or may not have controllable USB PHY. Quite often they don't. The
>>>>>>>> Baytrails have usually ULPI PHY for USB2, but that does not mean they
>>>>>>>> provide any vendor specific functions or any need for a driver in any
>>>>>>>> case.
>>>>>>>
>>>>>>> that's different from what I heard.
>>>>>>
>>>>>> I don't know where you got that impression, but it's not true. The
>>>>>> Baytrail SoCs for example don't have internal USB PHYs, which means
>>>>>> the manufacturers using it can select what they want. So we have
>>>>>> boards where PHY driver(s) is needed and boards where it isn't.
>>>>>
>>>>> alright, that explains it ;-) So you have external USB2 and USB3 PHYs ?
>>>>> You have an external PIPE3 interface ? That's quite an achievement,
>>>>> kudos to your HW designers. Getting timing closure on PIPE3 is a
>>>>> difficult task.
>>>>
>>>> No, only the USB2 PHY is external. I'm giving you wrong information,
>>>> I'm sorry about that. Need to concentrate on what I'm writing.
>>>>
>>>> <snip>
>>>>
>>>>>> This is really good to get. We have some projects where we are dealing
>>>>>> with more embedded environments, like IVI, where the kernel should be
>>>>>> stripped of everything useless. Since the PHYs are autonomous, we
>>>>>> should be able to disable the PHY libraries/frameworks.
>>>>>
>>>>> hmmm, in that case it's a lot easier to treat. We can use
>>>>> ERR_PTR(-ENXIO) as an indication that the framework is disabled, or
>>>>> something like that.
>>>>>
>>>>> The difficult is really reliably supporting e.g. OMAP5 (which won't work
>>>>> without a PHY) and your BayTrail with autonomous PHYs. What can we use
>>>>> as an indication ?
>>>>
>>>> OMAP has it's own glue driver, so shouldn't it depend on the PHY
>>>> layer?
>>>
>>> right, but the PHY is connected to the dwc3 core and not to the glue.
>>>>
>>>>> I mean, I need to know that a particular platform depends on a PHY
>>>>> driver before I decide to return -EPROBE_DEFER or just assume the PHY
>>>>> isn't needed ;-)
>>>>
>>>> I don't think dwc3 (core) should care about that. The PHY layer needs
>>>> to tell us that. If the PHY driver that the platform depends is not
>>>> available yet, the PHY layer returns -EPROBE_DEFER and dwc3 ends up
>>>> returning -EPROBE_DEFER.
>>>
>>> I don't think the PHY layer can 'reliably' tell if PHY driver is available or
>>> not. Consider when the phy_provider_register fails, there is no way to know if
>>> PHY driver is available or not. There are a few cases where PHY layer returns
>>> -EPROBE_DEFER but none of them can tell for sure that PHY driver is either
>>> available and failed or not available at all. It would be best for us to leave
>>> that to the platforms if we want to be sure if the platform needs a PHY or not.
>>>
>>
>> Just to summarize this thread on what we need
> 
> Thanks for summarizing.
>>
>> 1) dwc3 core shouldn't worry about platform specific stuff i.e. PHY needed or not.
>> It should be as generic as possible.
>>
>> 2) dwc3 core should continue probe even if PHY layer is not enabled, as not all platforms need it.
>>
>> 3) dwc3 core should continue probe if PHY device is not available. (-ENODEV?)
>>
>> 4) dwc3 core should error out on any error condition if PHY device is available and caused some error,
>> e.g. init error.
>>
>> 5) dwc3 core should return EPROBE_DEFER if PHY device is available but device driver is not yet loaded.
>>
>> 6) platform glue should do the necessary sanity checks for availability of all resources like PHY device, PHY layer, etc, before populating the dwc3 device. e.g. in OMAP5 case we could check if both usb2 and usb3 PHY
>> nodes are available in the DT and PHY layer is enabled, from dwc3-omap.c? In J6 case we could check that at least usb2 phy node is there for the High-Speed only controller, and so on.
> 
> The PHY is connected to the dwc3 core. So I'm not sure if we should be doing
> checks for PHY in the glue layer.

Sorry, I didn't get you. My reasoning was that since OMAP platform has this strict requirement of requiring
explicit PHY control in order to work, we must do the sanity checks in OMAP specific code and not in the dwc3 core code. It has nothing to do with how hardware is laid out.

What alternative do you suggest otherwise?

cheers,
-roger

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Kishon Vijay Abraham I]

Hi Roger,

On Friday 21 February 2014 05:59 PM, Roger Quadros wrote:
> On 02/21/2014 02:25 PM, Kishon Vijay Abraham I wrote:
>> Hi Roger,
>>
>> On Wednesday 19 February 2014 06:07 PM, Roger Quadros wrote:
>>> Hi,
>>>
>>> On 02/12/2014 11:46 AM, Kishon Vijay Abraham I wrote:
>>>> On Wednesday 29 January 2014 08:17 PM, Heikki Krogerus wrote:
>>>>> Hi,
>>>>>
>>>>> On Tue, Jan 28, 2014 at 10:30:36AM -0600, Felipe Balbi wrote:
>>>>>> On Tue, Jan 28, 2014 at 05:32:30PM +0200, Heikki Krogerus wrote:
>>>>>>> On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
>>>>>>> For the controller drivers the PHYs are just a resource like any
>>>>>>> other. The controller drivers can't have any responsibility of
>>>>>>> them. They should not care if PHY drivers are available for them or
>>>>>>> not, or even if the PHY framework is available or not.
>>>>>>
>>>>>> huh? If memory isn't available you don't continue probing, right ? If
>>>>>> your IORESOURCE_MEM is missing, you also don't continue probing, if your
>>>>>> IRQ line is missing, you bail too. Those are also nothing but resources
>>>>>> to the driver, what you're asking here is to treat PHY as a _different_
>>>>>> resource; which might be fine, but we need to make sure we don't
>>>>>> continue probing when a PHY is missing in a platform that certainly
>>>>>> needs a PHY.
>>>>>
>>>>> Yes, true. In my head I was comparing the PHY only to resources like
>>>>> gpios, clocks, dma channels, etc. that are often optional to the
>>>>> drivers.
>>>>>
>>>>>>>>>> But I really want to see the argument against using no-op. As far as I
>>>>>>>>>> could see, everybody needs a PHY driver one way or another, some
>>>>>>>>>> platforms just haven't sent any PHY driver upstream and have their own
>>>>>>>>>> hacked up solution to avoid using the PHY layer.
>>>>>>>>>
>>>>>>>>> Not true in our case. Platforms using Intel's SoCs and chip sets may
>>>>>>>>> or may not have controllable USB PHY. Quite often they don't. The
>>>>>>>>> Baytrails have usually ULPI PHY for USB2, but that does not mean they
>>>>>>>>> provide any vendor specific functions or any need for a driver in any
>>>>>>>>> case.
>>>>>>>>
>>>>>>>> that's different from what I heard.
>>>>>>>
>>>>>>> I don't know where you got that impression, but it's not true. The
>>>>>>> Baytrail SoCs for example don't have internal USB PHYs, which means
>>>>>>> the manufacturers using it can select what they want. So we have
>>>>>>> boards where PHY driver(s) is needed and boards where it isn't.
>>>>>>
>>>>>> alright, that explains it ;-) So you have external USB2 and USB3 PHYs ?
>>>>>> You have an external PIPE3 interface ? That's quite an achievement,
>>>>>> kudos to your HW designers. Getting timing closure on PIPE3 is a
>>>>>> difficult task.
>>>>>
>>>>> No, only the USB2 PHY is external. I'm giving you wrong information,
>>>>> I'm sorry about that. Need to concentrate on what I'm writing.
>>>>>
>>>>> <snip>
>>>>>
>>>>>>> This is really good to get. We have some projects where we are dealing
>>>>>>> with more embedded environments, like IVI, where the kernel should be
>>>>>>> stripped of everything useless. Since the PHYs are autonomous, we
>>>>>>> should be able to disable the PHY libraries/frameworks.
>>>>>>
>>>>>> hmmm, in that case it's a lot easier to treat. We can use
>>>>>> ERR_PTR(-ENXIO) as an indication that the framework is disabled, or
>>>>>> something like that.
>>>>>>
>>>>>> The difficult is really reliably supporting e.g. OMAP5 (which won't work
>>>>>> without a PHY) and your BayTrail with autonomous PHYs. What can we use
>>>>>> as an indication ?
>>>>>
>>>>> OMAP has it's own glue driver, so shouldn't it depend on the PHY
>>>>> layer?
>>>>
>>>> right, but the PHY is connected to the dwc3 core and not to the glue.
>>>>>
>>>>>> I mean, I need to know that a particular platform depends on a PHY
>>>>>> driver before I decide to return -EPROBE_DEFER or just assume the PHY
>>>>>> isn't needed ;-)
>>>>>
>>>>> I don't think dwc3 (core) should care about that. The PHY layer needs
>>>>> to tell us that. If the PHY driver that the platform depends is not
>>>>> available yet, the PHY layer returns -EPROBE_DEFER and dwc3 ends up
>>>>> returning -EPROBE_DEFER.
>>>>
>>>> I don't think the PHY layer can 'reliably' tell if PHY driver is available or
>>>> not. Consider when the phy_provider_register fails, there is no way to know if
>>>> PHY driver is available or not. There are a few cases where PHY layer returns
>>>> -EPROBE_DEFER but none of them can tell for sure that PHY driver is either
>>>> available and failed or not available at all. It would be best for us to leave
>>>> that to the platforms if we want to be sure if the platform needs a PHY or not.
>>>>
>>>
>>> Just to summarize this thread on what we need
>>
>> Thanks for summarizing.
>>>
>>> 1) dwc3 core shouldn't worry about platform specific stuff i.e. PHY needed or not.
>>> It should be as generic as possible.
>>>
>>> 2) dwc3 core should continue probe even if PHY layer is not enabled, as not all platforms need it.
>>>
>>> 3) dwc3 core should continue probe if PHY device is not available. (-ENODEV?)
>>>
>>> 4) dwc3 core should error out on any error condition if PHY device is available and caused some error,
>>> e.g. init error.
>>>
>>> 5) dwc3 core should return EPROBE_DEFER if PHY device is available but device driver is not yet loaded.
>>>
>>> 6) platform glue should do the necessary sanity checks for availability of all resources like PHY device, PHY layer, etc, before populating the dwc3 device. e.g. in OMAP5 case we could check if both usb2 and usb3 PHY
>>> nodes are available in the DT and PHY layer is enabled, from dwc3-omap.c? In J6 case we could check that at least usb2 phy node is there for the High-Speed only controller, and so on.
>>
>> The PHY is connected to the dwc3 core. So I'm not sure if we should be doing
>> checks for PHY in the glue layer.
> 
> Sorry, I didn't get you. My reasoning was that since OMAP platform has this strict requirement of requiring
> explicit PHY control in order to work, we must do the sanity checks in OMAP specific code and not in the dwc3 core code. It has nothing to do with how hardware is laid out.

What kind of sanity check do you think can be done in OMAP code? We don't use
any of the PHY API's in glue code. If we add the same PHY APIs in glue code it
will be duplication of the same code without much value besides breaking the
design guideline of the software to be modelled similar to hardware.

However in Kconfig of dwc3 glue we can add 'select GENERIC_PHY, select
PHY_OMAP_USB2, select OMAP_USB3' I guess.

Thanks
Kishon

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Roger Quadros]

On 02/24/2014 11:51 AM, Kishon Vijay Abraham I wrote:
> Hi Roger,
> 
> On Friday 21 February 2014 05:59 PM, Roger Quadros wrote:
>> On 02/21/2014 02:25 PM, Kishon Vijay Abraham I wrote:
>>> Hi Roger,
>>>
>>> On Wednesday 19 February 2014 06:07 PM, Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> On 02/12/2014 11:46 AM, Kishon Vijay Abraham I wrote:
>>>>> On Wednesday 29 January 2014 08:17 PM, Heikki Krogerus wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Tue, Jan 28, 2014 at 10:30:36AM -0600, Felipe Balbi wrote:
>>>>>>> On Tue, Jan 28, 2014 at 05:32:30PM +0200, Heikki Krogerus wrote:
>>>>>>>> On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
>>>>>>>> For the controller drivers the PHYs are just a resource like any
>>>>>>>> other. The controller drivers can't have any responsibility of
>>>>>>>> them. They should not care if PHY drivers are available for them or
>>>>>>>> not, or even if the PHY framework is available or not.
>>>>>>>
>>>>>>> huh? If memory isn't available you don't continue probing, right ? If
>>>>>>> your IORESOURCE_MEM is missing, you also don't continue probing, if your
>>>>>>> IRQ line is missing, you bail too. Those are also nothing but resources
>>>>>>> to the driver, what you're asking here is to treat PHY as a _different_
>>>>>>> resource; which might be fine, but we need to make sure we don't
>>>>>>> continue probing when a PHY is missing in a platform that certainly
>>>>>>> needs a PHY.
>>>>>>
>>>>>> Yes, true. In my head I was comparing the PHY only to resources like
>>>>>> gpios, clocks, dma channels, etc. that are often optional to the
>>>>>> drivers.
>>>>>>
>>>>>>>>>>> But I really want to see the argument against using no-op. As far as I
>>>>>>>>>>> could see, everybody needs a PHY driver one way or another, some
>>>>>>>>>>> platforms just haven't sent any PHY driver upstream and have their own
>>>>>>>>>>> hacked up solution to avoid using the PHY layer.
>>>>>>>>>>
>>>>>>>>>> Not true in our case. Platforms using Intel's SoCs and chip sets may
>>>>>>>>>> or may not have controllable USB PHY. Quite often they don't. The
>>>>>>>>>> Baytrails have usually ULPI PHY for USB2, but that does not mean they
>>>>>>>>>> provide any vendor specific functions or any need for a driver in any
>>>>>>>>>> case.
>>>>>>>>>
>>>>>>>>> that's different from what I heard.
>>>>>>>>
>>>>>>>> I don't know where you got that impression, but it's not true. The
>>>>>>>> Baytrail SoCs for example don't have internal USB PHYs, which means
>>>>>>>> the manufacturers using it can select what they want. So we have
>>>>>>>> boards where PHY driver(s) is needed and boards where it isn't.
>>>>>>>
>>>>>>> alright, that explains it ;-) So you have external USB2 and USB3 PHYs ?
>>>>>>> You have an external PIPE3 interface ? That's quite an achievement,
>>>>>>> kudos to your HW designers. Getting timing closure on PIPE3 is a
>>>>>>> difficult task.
>>>>>>
>>>>>> No, only the USB2 PHY is external. I'm giving you wrong information,
>>>>>> I'm sorry about that. Need to concentrate on what I'm writing.
>>>>>>
>>>>>> <snip>
>>>>>>
>>>>>>>> This is really good to get. We have some projects where we are dealing
>>>>>>>> with more embedded environments, like IVI, where the kernel should be
>>>>>>>> stripped of everything useless. Since the PHYs are autonomous, we
>>>>>>>> should be able to disable the PHY libraries/frameworks.
>>>>>>>
>>>>>>> hmmm, in that case it's a lot easier to treat. We can use
>>>>>>> ERR_PTR(-ENXIO) as an indication that the framework is disabled, or
>>>>>>> something like that.
>>>>>>>
>>>>>>> The difficult is really reliably supporting e.g. OMAP5 (which won't work
>>>>>>> without a PHY) and your BayTrail with autonomous PHYs. What can we use
>>>>>>> as an indication ?
>>>>>>
>>>>>> OMAP has it's own glue driver, so shouldn't it depend on the PHY
>>>>>> layer?
>>>>>
>>>>> right, but the PHY is connected to the dwc3 core and not to the glue.
>>>>>>
>>>>>>> I mean, I need to know that a particular platform depends on a PHY
>>>>>>> driver before I decide to return -EPROBE_DEFER or just assume the PHY
>>>>>>> isn't needed ;-)
>>>>>>
>>>>>> I don't think dwc3 (core) should care about that. The PHY layer needs
>>>>>> to tell us that. If the PHY driver that the platform depends is not
>>>>>> available yet, the PHY layer returns -EPROBE_DEFER and dwc3 ends up
>>>>>> returning -EPROBE_DEFER.
>>>>>
>>>>> I don't think the PHY layer can 'reliably' tell if PHY driver is available or
>>>>> not. Consider when the phy_provider_register fails, there is no way to know if
>>>>> PHY driver is available or not. There are a few cases where PHY layer returns
>>>>> -EPROBE_DEFER but none of them can tell for sure that PHY driver is either
>>>>> available and failed or not available at all. It would be best for us to leave
>>>>> that to the platforms if we want to be sure if the platform needs a PHY or not.
>>>>>
>>>>
>>>> Just to summarize this thread on what we need
>>>
>>> Thanks for summarizing.
>>>>
>>>> 1) dwc3 core shouldn't worry about platform specific stuff i.e. PHY needed or not.
>>>> It should be as generic as possible.
>>>>
>>>> 2) dwc3 core should continue probe even if PHY layer is not enabled, as not all platforms need it.
>>>>
>>>> 3) dwc3 core should continue probe if PHY device is not available. (-ENODEV?)
>>>>
>>>> 4) dwc3 core should error out on any error condition if PHY device is available and caused some error,
>>>> e.g. init error.
>>>>
>>>> 5) dwc3 core should return EPROBE_DEFER if PHY device is available but device driver is not yet loaded.
>>>>
>>>> 6) platform glue should do the necessary sanity checks for availability of all resources like PHY device, PHY layer, etc, before populating the dwc3 device. e.g. in OMAP5 case we could check if both usb2 and usb3 PHY
>>>> nodes are available in the DT and PHY layer is enabled, from dwc3-omap.c? In J6 case we could check that at least usb2 phy node is there for the High-Speed only controller, and so on.
>>>
>>> The PHY is connected to the dwc3 core. So I'm not sure if we should be doing
>>> checks for PHY in the glue layer.
>>
>> Sorry, I didn't get you. My reasoning was that since OMAP platform has this strict requirement of requiring
>> explicit PHY control in order to work, we must do the sanity checks in OMAP specific code and not in the dwc3 core code. It has nothing to do with how hardware is laid out.
> 
> What kind of sanity check do you think can be done in OMAP code? We don't use
> any of the PHY API's in glue code. If we add the same PHY APIs in glue code it
> will be duplication of the same code without much value besides breaking the
> design guideline of the software to be modelled similar to hardware.
> 

I wasn't saying about using PHY APIs in glue code, but just doing the basic sanity checks like
presence of PHY layer, the required USB PHY DT nodes and the required drivers for the platform.

> However in Kconfig of dwc3 glue we can add 'select GENERIC_PHY, select
> PHY_OMAP_USB2, select OMAP_USB3' I guess.

That won't be required if we can print to the user about the things we are missing for the device
to work.

cheers,
-roger

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Kishon Vijay Abraham I]

Hi,

On Monday 24 February 2014 04:35 PM, Roger Quadros wrote:
> On 02/24/2014 11:51 AM, Kishon Vijay Abraham I wrote:
>> Hi Roger,
>>
>> On Friday 21 February 2014 05:59 PM, Roger Quadros wrote:
>>> On 02/21/2014 02:25 PM, Kishon Vijay Abraham I wrote:
>>>> Hi Roger,
>>>>
>>>> On Wednesday 19 February 2014 06:07 PM, Roger Quadros wrote:
>>>>> Hi,
>>>>>
>>>>> On 02/12/2014 11:46 AM, Kishon Vijay Abraham I wrote:
>>>>>> On Wednesday 29 January 2014 08:17 PM, Heikki Krogerus wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Tue, Jan 28, 2014 at 10:30:36AM -0600, Felipe Balbi wrote:
>>>>>>>> On Tue, Jan 28, 2014 at 05:32:30PM +0200, Heikki Krogerus wrote:
>>>>>>>>> On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
>>>>>>>>> For the controller drivers the PHYs are just a resource like any
>>>>>>>>> other. The controller drivers can't have any responsibility of
>>>>>>>>> them. They should not care if PHY drivers are available for them or
>>>>>>>>> not, or even if the PHY framework is available or not.
>>>>>>>>
>>>>>>>> huh? If memory isn't available you don't continue probing, right ? If
>>>>>>>> your IORESOURCE_MEM is missing, you also don't continue probing, if your
>>>>>>>> IRQ line is missing, you bail too. Those are also nothing but resources
>>>>>>>> to the driver, what you're asking here is to treat PHY as a _different_
>>>>>>>> resource; which might be fine, but we need to make sure we don't
>>>>>>>> continue probing when a PHY is missing in a platform that certainly
>>>>>>>> needs a PHY.
>>>>>>>
>>>>>>> Yes, true. In my head I was comparing the PHY only to resources like
>>>>>>> gpios, clocks, dma channels, etc. that are often optional to the
>>>>>>> drivers.
>>>>>>>
>>>>>>>>>>>> But I really want to see the argument against using no-op. As far as I
>>>>>>>>>>>> could see, everybody needs a PHY driver one way or another, some
>>>>>>>>>>>> platforms just haven't sent any PHY driver upstream and have their own
>>>>>>>>>>>> hacked up solution to avoid using the PHY layer.
>>>>>>>>>>>
>>>>>>>>>>> Not true in our case. Platforms using Intel's SoCs and chip sets may
>>>>>>>>>>> or may not have controllable USB PHY. Quite often they don't. The
>>>>>>>>>>> Baytrails have usually ULPI PHY for USB2, but that does not mean they
>>>>>>>>>>> provide any vendor specific functions or any need for a driver in any
>>>>>>>>>>> case.
>>>>>>>>>>
>>>>>>>>>> that's different from what I heard.
>>>>>>>>>
>>>>>>>>> I don't know where you got that impression, but it's not true. The
>>>>>>>>> Baytrail SoCs for example don't have internal USB PHYs, which means
>>>>>>>>> the manufacturers using it can select what they want. So we have
>>>>>>>>> boards where PHY driver(s) is needed and boards where it isn't.
>>>>>>>>
>>>>>>>> alright, that explains it ;-) So you have external USB2 and USB3 PHYs ?
>>>>>>>> You have an external PIPE3 interface ? That's quite an achievement,
>>>>>>>> kudos to your HW designers. Getting timing closure on PIPE3 is a
>>>>>>>> difficult task.
>>>>>>>
>>>>>>> No, only the USB2 PHY is external. I'm giving you wrong information,
>>>>>>> I'm sorry about that. Need to concentrate on what I'm writing.
>>>>>>>
>>>>>>> <snip>
>>>>>>>
>>>>>>>>> This is really good to get. We have some projects where we are dealing
>>>>>>>>> with more embedded environments, like IVI, where the kernel should be
>>>>>>>>> stripped of everything useless. Since the PHYs are autonomous, we
>>>>>>>>> should be able to disable the PHY libraries/frameworks.
>>>>>>>>
>>>>>>>> hmmm, in that case it's a lot easier to treat. We can use
>>>>>>>> ERR_PTR(-ENXIO) as an indication that the framework is disabled, or
>>>>>>>> something like that.
>>>>>>>>
>>>>>>>> The difficult is really reliably supporting e.g. OMAP5 (which won't work
>>>>>>>> without a PHY) and your BayTrail with autonomous PHYs. What can we use
>>>>>>>> as an indication ?
>>>>>>>
>>>>>>> OMAP has it's own glue driver, so shouldn't it depend on the PHY
>>>>>>> layer?
>>>>>>
>>>>>> right, but the PHY is connected to the dwc3 core and not to the glue.
>>>>>>>
>>>>>>>> I mean, I need to know that a particular platform depends on a PHY
>>>>>>>> driver before I decide to return -EPROBE_DEFER or just assume the PHY
>>>>>>>> isn't needed ;-)
>>>>>>>
>>>>>>> I don't think dwc3 (core) should care about that. The PHY layer needs
>>>>>>> to tell us that. If the PHY driver that the platform depends is not
>>>>>>> available yet, the PHY layer returns -EPROBE_DEFER and dwc3 ends up
>>>>>>> returning -EPROBE_DEFER.
>>>>>>
>>>>>> I don't think the PHY layer can 'reliably' tell if PHY driver is available or
>>>>>> not. Consider when the phy_provider_register fails, there is no way to know if
>>>>>> PHY driver is available or not. There are a few cases where PHY layer returns
>>>>>> -EPROBE_DEFER but none of them can tell for sure that PHY driver is either
>>>>>> available and failed or not available at all. It would be best for us to leave
>>>>>> that to the platforms if we want to be sure if the platform needs a PHY or not.
>>>>>>
>>>>>
>>>>> Just to summarize this thread on what we need
>>>>
>>>> Thanks for summarizing.
>>>>>
>>>>> 1) dwc3 core shouldn't worry about platform specific stuff i.e. PHY needed or not.
>>>>> It should be as generic as possible.
>>>>>
>>>>> 2) dwc3 core should continue probe even if PHY layer is not enabled, as not all platforms need it.
>>>>>
>>>>> 3) dwc3 core should continue probe if PHY device is not available. (-ENODEV?)
>>>>>
>>>>> 4) dwc3 core should error out on any error condition if PHY device is available and caused some error,
>>>>> e.g. init error.
>>>>>
>>>>> 5) dwc3 core should return EPROBE_DEFER if PHY device is available but device driver is not yet loaded.
>>>>>
>>>>> 6) platform glue should do the necessary sanity checks for availability of all resources like PHY device, PHY layer, etc, before populating the dwc3 device. e.g. in OMAP5 case we could check if both usb2 and usb3 PHY
>>>>> nodes are available in the DT and PHY layer is enabled, from dwc3-omap.c? In J6 case we could check that at least usb2 phy node is there for the High-Speed only controller, and so on.
>>>>
>>>> The PHY is connected to the dwc3 core. So I'm not sure if we should be doing
>>>> checks for PHY in the glue layer.
>>>
>>> Sorry, I didn't get you. My reasoning was that since OMAP platform has this strict requirement of requiring
>>> explicit PHY control in order to work, we must do the sanity checks in OMAP specific code and not in the dwc3 core code. It has nothing to do with how hardware is laid out.
>>
>> What kind of sanity check do you think can be done in OMAP code? We don't use
>> any of the PHY API's in glue code. If we add the same PHY APIs in glue code it
>> will be duplication of the same code without much value besides breaking the
>> design guideline of the software to be modelled similar to hardware.
>>
> 
> I wasn't saying about using PHY APIs in glue code, but just doing the basic sanity checks like
> presence of PHY layer, the required USB PHY DT nodes and the required drivers for the platform.

hmm.. instead of doing sanity checks we can just select them in Kconfig like
what I suggested before no? We could check if there is a phandle on the child
dwc3 node though.

Thanks
Kishon

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Felipe Balbi]

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

On Mon, Feb 24, 2014 at 03:21:05PM +0530, Kishon Vijay Abraham I wrote:
> Hi Roger,
> 
> On Friday 21 February 2014 05:59 PM, Roger Quadros wrote:
> > On 02/21/2014 02:25 PM, Kishon Vijay Abraham I wrote:
> >> Hi Roger,
> >>
> >> On Wednesday 19 February 2014 06:07 PM, Roger Quadros wrote:
> >>> Hi,
> >>>
> >>> On 02/12/2014 11:46 AM, Kishon Vijay Abraham I wrote:
> >>>> On Wednesday 29 January 2014 08:17 PM, Heikki Krogerus wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Tue, Jan 28, 2014 at 10:30:36AM -0600, Felipe Balbi wrote:
> >>>>>> On Tue, Jan 28, 2014 at 05:32:30PM +0200, Heikki Krogerus wrote:
> >>>>>>> On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
> >>>>>>> For the controller drivers the PHYs are just a resource like any
> >>>>>>> other. The controller drivers can't have any responsibility of
> >>>>>>> them. They should not care if PHY drivers are available for them or
> >>>>>>> not, or even if the PHY framework is available or not.
> >>>>>>
> >>>>>> huh? If memory isn't available you don't continue probing, right ? If
> >>>>>> your IORESOURCE_MEM is missing, you also don't continue probing, if your
> >>>>>> IRQ line is missing, you bail too. Those are also nothing but resources
> >>>>>> to the driver, what you're asking here is to treat PHY as a _different_
> >>>>>> resource; which might be fine, but we need to make sure we don't
> >>>>>> continue probing when a PHY is missing in a platform that certainly
> >>>>>> needs a PHY.
> >>>>>
> >>>>> Yes, true. In my head I was comparing the PHY only to resources like
> >>>>> gpios, clocks, dma channels, etc. that are often optional to the
> >>>>> drivers.
> >>>>>
> >>>>>>>>>> But I really want to see the argument against using no-op. As far as I
> >>>>>>>>>> could see, everybody needs a PHY driver one way or another, some
> >>>>>>>>>> platforms just haven't sent any PHY driver upstream and have their own
> >>>>>>>>>> hacked up solution to avoid using the PHY layer.
> >>>>>>>>>
> >>>>>>>>> Not true in our case. Platforms using Intel's SoCs and chip sets may
> >>>>>>>>> or may not have controllable USB PHY. Quite often they don't. The
> >>>>>>>>> Baytrails have usually ULPI PHY for USB2, but that does not mean they
> >>>>>>>>> provide any vendor specific functions or any need for a driver in any
> >>>>>>>>> case.
> >>>>>>>>
> >>>>>>>> that's different from what I heard.
> >>>>>>>
> >>>>>>> I don't know where you got that impression, but it's not true. The
> >>>>>>> Baytrail SoCs for example don't have internal USB PHYs, which means
> >>>>>>> the manufacturers using it can select what they want. So we have
> >>>>>>> boards where PHY driver(s) is needed and boards where it isn't.
> >>>>>>
> >>>>>> alright, that explains it ;-) So you have external USB2 and USB3 PHYs ?
> >>>>>> You have an external PIPE3 interface ? That's quite an achievement,
> >>>>>> kudos to your HW designers. Getting timing closure on PIPE3 is a
> >>>>>> difficult task.
> >>>>>
> >>>>> No, only the USB2 PHY is external. I'm giving you wrong information,
> >>>>> I'm sorry about that. Need to concentrate on what I'm writing.
> >>>>>
> >>>>> <snip>
> >>>>>
> >>>>>>> This is really good to get. We have some projects where we are dealing
> >>>>>>> with more embedded environments, like IVI, where the kernel should be
> >>>>>>> stripped of everything useless. Since the PHYs are autonomous, we
> >>>>>>> should be able to disable the PHY libraries/frameworks.
> >>>>>>
> >>>>>> hmmm, in that case it's a lot easier to treat. We can use
> >>>>>> ERR_PTR(-ENXIO) as an indication that the framework is disabled, or
> >>>>>> something like that.
> >>>>>>
> >>>>>> The difficult is really reliably supporting e.g. OMAP5 (which won't work
> >>>>>> without a PHY) and your BayTrail with autonomous PHYs. What can we use
> >>>>>> as an indication ?
> >>>>>
> >>>>> OMAP has it's own glue driver, so shouldn't it depend on the PHY
> >>>>> layer?
> >>>>
> >>>> right, but the PHY is connected to the dwc3 core and not to the glue.
> >>>>>
> >>>>>> I mean, I need to know that a particular platform depends on a PHY
> >>>>>> driver before I decide to return -EPROBE_DEFER or just assume the PHY
> >>>>>> isn't needed ;-)
> >>>>>
> >>>>> I don't think dwc3 (core) should care about that. The PHY layer needs
> >>>>> to tell us that. If the PHY driver that the platform depends is not
> >>>>> available yet, the PHY layer returns -EPROBE_DEFER and dwc3 ends up
> >>>>> returning -EPROBE_DEFER.
> >>>>
> >>>> I don't think the PHY layer can 'reliably' tell if PHY driver is available or
> >>>> not. Consider when the phy_provider_register fails, there is no way to know if
> >>>> PHY driver is available or not. There are a few cases where PHY layer returns
> >>>> -EPROBE_DEFER but none of them can tell for sure that PHY driver is either
> >>>> available and failed or not available at all. It would be best for us to leave
> >>>> that to the platforms if we want to be sure if the platform needs a PHY or not.
> >>>>
> >>>
> >>> Just to summarize this thread on what we need
> >>
> >> Thanks for summarizing.
> >>>
> >>> 1) dwc3 core shouldn't worry about platform specific stuff i.e.
> >>> PHY needed or not.  It should be as generic as possible.
> >>>
> >>> 2) dwc3 core should continue probe even if PHY layer is not
> >>> enabled, as not all platforms need it.
> >>>
> >>> 3) dwc3 core should continue probe if PHY device is not available.
> >>> (-ENODEV?)
> >>>
> >>> 4) dwc3 core should error out on any error condition if PHY device
> >>> is available and caused some error, e.g. init error.
> >>>
> >>> 5) dwc3 core should return EPROBE_DEFER if PHY device is available
> >>> but device driver is not yet loaded.
> >>>
> >>> 6) platform glue should do the necessary sanity checks for
> >>> availability of all resources like PHY device, PHY layer, etc,
> >>> before populating the dwc3 device. e.g. in OMAP5 case we could
> >>> check if both usb2 and usb3 PHY nodes are available in the DT and
> >>> PHY layer is enabled, from dwc3-omap.c? In J6 case we could check
> >>> that at least usb2 phy node is there for the High-Speed only
> >>> controller, and so on.
> >>
> >> The PHY is connected to the dwc3 core. So I'm not sure if we should
> >> be doing checks for PHY in the glue layer.
> > 
> > Sorry, I didn't get you. My reasoning was that since OMAP platform
> > has this strict requirement of requiring explicit PHY control in
> > order to work, we must do the sanity checks in OMAP specific code
> > and not in the dwc3 core code. It has nothing to do with how
> > hardware is laid out.
> 
> What kind of sanity check do you think can be done in OMAP code? We don't use
> any of the PHY API's in glue code. If we add the same PHY APIs in glue code it
> will be duplication of the same code without much value besides breaking the
> design guideline of the software to be modelled similar to hardware.
> 
> However in Kconfig of dwc3 glue we can add 'select GENERIC_PHY, select
> PHY_OMAP_USB2, select OMAP_USB3' I guess.

ick, please don't. We have suffered too much from selects being
sprinkled all over the place.

-- 
balbi

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

Re: [PATCH 1/2] usb: dwc3: core: continue probing if usb phy library returns -ENODEV/-ENXIO

[Kishon Vijay Abraham I]

On Friday 21 February 2014 05:59 PM, Roger Quadros wrote:
> On 02/21/2014 02:25 PM, Kishon Vijay Abraham I wrote:
>> Hi Roger,
>>
>> On Wednesday 19 February 2014 06:07 PM, Roger Quadros wrote:
>>> Hi,
>>>
>>> On 02/12/2014 11:46 AM, Kishon Vijay Abraham I wrote:
>>>> On Wednesday 29 January 2014 08:17 PM, Heikki Krogerus wrote:
>>>>> Hi,
>>>>>
>>>>> On Tue, Jan 28, 2014 at 10:30:36AM -0600, Felipe Balbi wrote:
>>>>>> On Tue, Jan 28, 2014 at 05:32:30PM +0200, Heikki Krogerus wrote:
>>>>>>> On Mon, Jan 27, 2014 at 10:05:20AM -0600, Felipe Balbi wrote:
>>>>>>> For the controller drivers the PHYs are just a resource like any
>>>>>>> other. The controller drivers can't have any responsibility of
>>>>>>> them. They should not care if PHY drivers are available for them or
>>>>>>> not, or even if the PHY framework is available or not.
>>>>>>
>>>>>> huh? If memory isn't available you don't continue probing, right ? If
>>>>>> your IORESOURCE_MEM is missing, you also don't continue probing, if your
>>>>>> IRQ line is missing, you bail too. Those are also nothing but resources
>>>>>> to the driver, what you're asking here is to treat PHY as a _different_
>>>>>> resource; which might be fine, but we need to make sure we don't
>>>>>> continue probing when a PHY is missing in a platform that certainly
>>>>>> needs a PHY.
>>>>>
>>>>> Yes, true. In my head I was comparing the PHY only to resources like
>>>>> gpios, clocks, dma channels, etc. that are often optional to the
>>>>> drivers.
>>>>>
>>>>>>>>>> But I really want to see the argument against using no-op. As far as I
>>>>>>>>>> could see, everybody needs a PHY driver one way or another, some
>>>>>>>>>> platforms just haven't sent any PHY driver upstream and have their own
>>>>>>>>>> hacked up solution to avoid using the PHY layer.
>>>>>>>>>
>>>>>>>>> Not true in our case. Platforms using Intel's SoCs and chip sets may
>>>>>>>>> or may not have controllable USB PHY. Quite often they don't. The
>>>>>>>>> Baytrails have usually ULPI PHY for USB2, but that does not mean they
>>>>>>>>> provide any vendor specific functions or any need for a driver in any
>>>>>>>>> case.
>>>>>>>>
>>>>>>>> that's different from what I heard.
>>>>>>>
>>>>>>> I don't know where you got that impression, but it's not true. The
>>>>>>> Baytrail SoCs for example don't have internal USB PHYs, which means
>>>>>>> the manufacturers using it can select what they want. So we have
>>>>>>> boards where PHY driver(s) is needed and boards where it isn't.
>>>>>>
>>>>>> alright, that explains it ;-) So you have external USB2 and USB3 PHYs ?
>>>>>> You have an external PIPE3 interface ? That's quite an achievement,
>>>>>> kudos to your HW designers. Getting timing closure on PIPE3 is a
>>>>>> difficult task.
>>>>>
>>>>> No, only the USB2 PHY is external. I'm giving you wrong information,
>>>>> I'm sorry about that. Need to concentrate on what I'm writing.
>>>>>
>>>>> <snip>
>>>>>
>>>>>>> This is really good to get. We have some projects where we are dealing
>>>>>>> with more embedded environments, like IVI, where the kernel should be
>>>>>>> stripped of everything useless. Since the PHYs are autonomous, we
>>>>>>> should be able to disable the PHY libraries/frameworks.
>>>>>>
>>>>>> hmmm, in that case it's a lot easier to treat. We can use
>>>>>> ERR_PTR(-ENXIO) as an indication that the framework is disabled, or
>>>>>> something like that.
>>>>>>
>>>>>> The difficult is really reliably supporting e.g. OMAP5 (which won't work
>>>>>> without a PHY) and your BayTrail with autonomous PHYs. What can we use
>>>>>> as an indication ?
>>>>>
>>>>> OMAP has it's own glue driver, so shouldn't it depend on the PHY
>>>>> layer?
>>>>
>>>> right, but the PHY is connected to the dwc3 core and not to the glue.
>>>>>
>>>>>> I mean, I need to know that a particular platform depends on a PHY
>>>>>> driver before I decide to return -EPROBE_DEFER or just assume the PHY
>>>>>> isn't needed ;-)
>>>>>
>>>>> I don't think dwc3 (core) should care about that. The PHY layer needs
>>>>> to tell us that. If the PHY driver that the platform depends is not
>>>>> available yet, the PHY layer returns -EPROBE_DEFER and dwc3 ends up
>>>>> returning -EPROBE_DEFER.
>>>>
>>>> I don't think the PHY layer can 'reliably' tell if PHY driver is available or
>>>> not. Consider when the phy_provider_register fails, there is no way to know if
>>>> PHY driver is available or not. There are a few cases where PHY layer returns
>>>> -EPROBE_DEFER but none of them can tell for sure that PHY driver is either
>>>> available and failed or not available at all. It would be best for us to leave
>>>> that to the platforms if we want to be sure if the platform needs a PHY or not.
>>>>
>>>
>>> Just to summarize this thread on what we need
>>
>> Thanks for summarizing.
>>>
>>> 1) dwc3 core shouldn't worry about platform specific stuff i.e. PHY needed or not.
>>> It should be as generic as possible.

I think this contradicts with Felipe's requirement of
dwc3 core bailing out if a particular platform needs a PHY but it's not able to
get it.
>>>
>>> 2) dwc3 core should continue probe even if PHY layer is not enabled, as not all platforms need it.
>>>
>>> 3) dwc3 core should continue probe if PHY device is not available. (-ENODEV?)
>>>
>>> 4) dwc3 core should error out on any error condition if PHY device is available and caused some error,
>>> e.g. init error.
>>>
>>> 5) dwc3 core should return EPROBE_DEFER if PHY device is available but device driver is not yet loaded.
>>>
>>> 6) platform glue should do the necessary sanity checks for availability of all resources like PHY device, PHY layer, etc, before populating the dwc3 device. e.g. in OMAP5 case we could check if both usb2 and usb3 PHY
>>> nodes are available in the DT and PHY layer is enabled, from dwc3-omap.c? In J6 case we could check that at least usb2 phy node is there for the High-Speed only controller, and so on.
>>
>> The PHY is connected to the dwc3 core. So I'm not sure if we should be doing
>> checks for PHY in the glue layer.
> 
> Sorry, I didn't get you. My reasoning was that since OMAP platform has this strict requirement of requiring
> explicit PHY control in order to work, we must do the sanity checks in OMAP specific code and not in the dwc3 core code. It has nothing to do with how hardware is laid out.
> 
> What alternative do you suggest otherwise?

Thanks
Kishon