[RESEND PATCH v2 0/7] usb: xhci-plat: support generic PHY and vbus regulator

[RESEND PATCH v2 0/7] usb: xhci-plat: support generic PHY and vbus regulator

[Jisheng Zhang]

The Marvell BG4CT has xhci controller. This controller has two phys:
one for usb2 and another for usb3. BG4CT boards have board level vbus
control through gpio.

I plan to add the xhci support in two steps: first of all, add generic
PHY and vbus regulator control support to the xhci-plat driver. Then
add the usb2 and usb3 phy drivers, after that, we add the phy and xhci
nodes in the dtsi.

This series takes the first step. The first three patches are bug fix.
Then two clean up patches. The last two patches add generic PHY and
vbus regulator control support.

Since v1:
 - fix NULL pointer dereference in [PATCH 7/7]

Jisheng Zhang (7):
  usb: xhci: plat: Fix suspend/resume when the optional clk exists
  usb: xhci: plat: attach the usb_phy to the correct hcd
  usb: xhci: plat: Fix suspend/resume when the optional usb_phy exists
  usb: xhci: plat: sort the headers in alphabetic order
  usb: xhci: plat: Remove checks for optional clock in error/remove path
  usb: xhci: plat: add generic PHY support
  usb: xhci: plat: add vbus regulator control

 drivers/usb/host/xhci-plat.c | 162 +++++++++++++++++++++++++++++++++++++------
 drivers/usb/host/xhci.h      |   2 +
 2 files changed, 142 insertions(+), 22 deletions(-)

-- 
2.8.1

[RESEND PATCH v2 0/7] usb: xhci-plat: support generic PHY and vbus regulator

[Jisheng Zhang]

The Marvell BG4CT has xhci controller. This controller has two phys:
one for usb2 and another for usb3. BG4CT boards have board level vbus
control through gpio.

I plan to add the xhci support in two steps: first of all, add generic
PHY and vbus regulator control support to the xhci-plat driver. Then
add the usb2 and usb3 phy drivers, after that, we add the phy and xhci
nodes in the dtsi.

This series takes the first step. The first three patches are bug fix.
Then two clean up patches. The last two patches add generic PHY and
vbus regulator control support.

Since v1:
 - fix NULL pointer dereference in [PATCH 7/7]

Jisheng Zhang (7):
  usb: xhci: plat: Fix suspend/resume when the optional clk exists
  usb: xhci: plat: attach the usb_phy to the correct hcd
  usb: xhci: plat: Fix suspend/resume when the optional usb_phy exists
  usb: xhci: plat: sort the headers in alphabetic order
  usb: xhci: plat: Remove checks for optional clock in error/remove path
  usb: xhci: plat: add generic PHY support
  usb: xhci: plat: add vbus regulator control

 drivers/usb/host/xhci-plat.c | 162 +++++++++++++++++++++++++++++++++++++------
 drivers/usb/host/xhci.h      |   2 +
 2 files changed, 142 insertions(+), 22 deletions(-)

-- 
2.8.1

[RESEND PATCH v2 1/7] usb: xhci: plat: Fix suspend/resume when the optional clk exists

[Jisheng Zhang]

Commit 4718c1774051 ("usb: host: xhci-plat: add clock support") adds
optional clk support, but it forgets to prepare/disable and
enable/unprepare the clk in the resume/suspend path. This path fixes
this issue by adding missing clk related calls.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
Fixes: 4718c1774051 ("usb: host: xhci-plat: add clock support")
---
 drivers/usb/host/xhci-plat.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 474b5fa..8cb46cb 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -295,6 +295,7 @@ static int xhci_plat_remove(struct platform_device *dev)
 #ifdef CONFIG_PM_SLEEP
 static int xhci_plat_suspend(struct device *dev)
 {
+	int ret;
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 
@@ -306,14 +307,25 @@ static int xhci_plat_suspend(struct device *dev)
 	 * reconsider this when xhci_plat_suspend enlarges its scope, e.g.,
 	 * also applies to runtime suspend.
 	 */
-	return xhci_suspend(xhci, device_may_wakeup(dev));
+	ret = xhci_suspend(xhci, device_may_wakeup(dev));
+	if (ret)
+		return ret;
+
+	clk_disable_unprepare(xhci->clk);
+
+	return ret;
 }
 
 static int xhci_plat_resume(struct device *dev)
 {
+	int ret;
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 
+	ret = clk_prepare_enable(xhci->clk);
+	if (ret)
+		return ret;
+
 	return xhci_resume(xhci, 0);
 }
 
-- 
2.8.1

[RESEND PATCH v2 1/7] usb: xhci: plat: Fix suspend/resume when the optional clk exists

[Jisheng Zhang]

Commit 4718c1774051 ("usb: host: xhci-plat: add clock support") adds
optional clk support, but it forgets to prepare/disable and
enable/unprepare the clk in the resume/suspend path. This path fixes
this issue by adding missing clk related calls.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
Fixes: 4718c1774051 ("usb: host: xhci-plat: add clock support")
---
 drivers/usb/host/xhci-plat.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 474b5fa..8cb46cb 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -295,6 +295,7 @@ static int xhci_plat_remove(struct platform_device *dev)
 #ifdef CONFIG_PM_SLEEP
 static int xhci_plat_suspend(struct device *dev)
 {
+	int ret;
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 
@@ -306,14 +307,25 @@ static int xhci_plat_suspend(struct device *dev)
 	 * reconsider this when xhci_plat_suspend enlarges its scope, e.g.,
 	 * also applies to runtime suspend.
 	 */
-	return xhci_suspend(xhci, device_may_wakeup(dev));
+	ret = xhci_suspend(xhci, device_may_wakeup(dev));
+	if (ret)
+		return ret;
+
+	clk_disable_unprepare(xhci->clk);
+
+	return ret;
 }
 
 static int xhci_plat_resume(struct device *dev)
 {
+	int ret;
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 
+	ret = clk_prepare_enable(xhci->clk);
+	if (ret)
+		return ret;
+
 	return xhci_resume(xhci, 0);
 }
 
-- 
2.8.1

[RESEND PATCH v2 2/7] usb: xhci: plat: attach the usb_phy to the correct hcd

[Jisheng Zhang]

Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") adds the
usb_phy for usb3, but it attached the usb_phy to incorrect hcd. The
xhci->shared_hcd is the hcd for usb3, this patch fixes this issue
by attach the usb_phy to the xhci->shared_hcd.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/usb/host/xhci-plat.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 8cb46cb..9ff89e9 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -144,6 +144,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	struct resource         *res;
 	struct usb_hcd		*hcd;
 	struct clk              *clk;
+	struct usb_phy		*usb_phy;
 	int			ret;
 	int			irq;
 
@@ -231,17 +232,18 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
 		xhci->shared_hcd->can_do_streams = 1;
 
-	hcd->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
-	if (IS_ERR(hcd->usb_phy)) {
-		ret = PTR_ERR(hcd->usb_phy);
+	usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
+	if (IS_ERR(usb_phy)) {
+		ret = PTR_ERR(usb_phy);
 		if (ret == -EPROBE_DEFER)
 			goto put_usb3_hcd;
-		hcd->usb_phy = NULL;
+		usb_phy = NULL;
 	} else {
-		ret = usb_phy_init(hcd->usb_phy);
+		ret = usb_phy_init(usb_phy);
 		if (ret)
 			goto put_usb3_hcd;
 	}
+	xhci->shared_hcd->usb_phy = usb_phy;
 
 	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (ret)
@@ -258,7 +260,7 @@ dealloc_usb2_hcd:
 	usb_remove_hcd(hcd);
 
 disable_usb_phy:
-	usb_phy_shutdown(hcd->usb_phy);
+	usb_phy_shutdown(usb_phy);
 
 put_usb3_hcd:
 	usb_put_hcd(xhci->shared_hcd);
@@ -280,7 +282,7 @@ static int xhci_plat_remove(struct platform_device *dev)
 	struct clk *clk = xhci->clk;
 
 	usb_remove_hcd(xhci->shared_hcd);
-	usb_phy_shutdown(hcd->usb_phy);
+	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
 
 	usb_remove_hcd(hcd);
 	usb_put_hcd(xhci->shared_hcd);
-- 
2.8.1

[RESEND PATCH v2 2/7] usb: xhci: plat: attach the usb_phy to the correct hcd

[Jisheng Zhang]

Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") adds the
usb_phy for usb3, but it attached the usb_phy to incorrect hcd. The
xhci->shared_hcd is the hcd for usb3, this patch fixes this issue
by attach the usb_phy to the xhci->shared_hcd.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/usb/host/xhci-plat.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 8cb46cb..9ff89e9 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -144,6 +144,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	struct resource         *res;
 	struct usb_hcd		*hcd;
 	struct clk              *clk;
+	struct usb_phy		*usb_phy;
 	int			ret;
 	int			irq;
 
@@ -231,17 +232,18 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
 		xhci->shared_hcd->can_do_streams = 1;
 
-	hcd->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
-	if (IS_ERR(hcd->usb_phy)) {
-		ret = PTR_ERR(hcd->usb_phy);
+	usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
+	if (IS_ERR(usb_phy)) {
+		ret = PTR_ERR(usb_phy);
 		if (ret == -EPROBE_DEFER)
 			goto put_usb3_hcd;
-		hcd->usb_phy = NULL;
+		usb_phy = NULL;
 	} else {
-		ret = usb_phy_init(hcd->usb_phy);
+		ret = usb_phy_init(usb_phy);
 		if (ret)
 			goto put_usb3_hcd;
 	}
+	xhci->shared_hcd->usb_phy = usb_phy;
 
 	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (ret)
@@ -258,7 +260,7 @@ dealloc_usb2_hcd:
 	usb_remove_hcd(hcd);
 
 disable_usb_phy:
-	usb_phy_shutdown(hcd->usb_phy);
+	usb_phy_shutdown(usb_phy);
 
 put_usb3_hcd:
 	usb_put_hcd(xhci->shared_hcd);
@@ -280,7 +282,7 @@ static int xhci_plat_remove(struct platform_device *dev)
 	struct clk *clk = xhci->clk;
 
 	usb_remove_hcd(xhci->shared_hcd);
-	usb_phy_shutdown(hcd->usb_phy);
+	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
 
 	usb_remove_hcd(hcd);
 	usb_put_hcd(xhci->shared_hcd);
-- 
2.8.1

[RESEND PATCH v2 3/7] usb: xhci: plat: Fix suspend/resume when the optional usb_phy exists

[Jisheng Zhang]

Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") adds the
usb_phy for usb3, but it forgets to shutdown/init the usb_phy in the
suspend/resume path. This patch fixes this issue by adding missing
usb_phy related calls.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/usb/host/xhci-plat.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 9ff89e9..fbd23fd 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -313,6 +313,7 @@ static int xhci_plat_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
+	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
 	clk_disable_unprepare(xhci->clk);
 
 	return ret;
@@ -328,6 +329,10 @@ static int xhci_plat_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	ret = usb_phy_init(xhci->shared_hcd->usb_phy);
+	if (ret)
+		return ret;
+
 	return xhci_resume(xhci, 0);
 }
 
-- 
2.8.1

[RESEND PATCH v2 3/7] usb: xhci: plat: Fix suspend/resume when the optional usb_phy exists

[Jisheng Zhang]

Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") adds the
usb_phy for usb3, but it forgets to shutdown/init the usb_phy in the
suspend/resume path. This patch fixes this issue by adding missing
usb_phy related calls.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/usb/host/xhci-plat.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 9ff89e9..fbd23fd 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -313,6 +313,7 @@ static int xhci_plat_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
+	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
 	clk_disable_unprepare(xhci->clk);
 
 	return ret;
@@ -328,6 +329,10 @@ static int xhci_plat_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	ret = usb_phy_init(xhci->shared_hcd->usb_phy);
+	if (ret)
+		return ret;
+
 	return xhci_resume(xhci, 0);
 }
 
-- 
2.8.1

[RESEND PATCH v2 4/7] usb: xhci: plat: sort the headers in alphabetic order

[Jisheng Zhang]

Sorting the headers in alphabetic order will help to reduce the conflict
when adding new headers later.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/usb/host/xhci-plat.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index fbd23fd..0e69712 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -11,19 +11,19 @@
  * version 2 as published by the Free Software Foundation.
  */
 
+#include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <linux/usb/phy.h>
 #include <linux/slab.h>
+#include <linux/usb/phy.h>
 #include <linux/usb/xhci_pdriver.h>
-#include <linux/acpi.h>
 
 #include "xhci.h"
-#include "xhci-plat.h"
 #include "xhci-mvebu.h"
+#include "xhci-plat.h"
 #include "xhci-rcar.h"
 
 static struct hc_driver __read_mostly xhci_plat_hc_driver;
-- 
2.8.1

[RESEND PATCH v2 4/7] usb: xhci: plat: sort the headers in alphabetic order

[Jisheng Zhang]

Sorting the headers in alphabetic order will help to reduce the conflict
when adding new headers later.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/usb/host/xhci-plat.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index fbd23fd..0e69712 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -11,19 +11,19 @@
  * version 2 as published by the Free Software Foundation.
  */
 
+#include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <linux/usb/phy.h>
 #include <linux/slab.h>
+#include <linux/usb/phy.h>
 #include <linux/usb/xhci_pdriver.h>
-#include <linux/acpi.h>
 
 #include "xhci.h"
-#include "xhci-plat.h"
 #include "xhci-mvebu.h"
+#include "xhci-plat.h"
 #include "xhci-rcar.h"
 
 static struct hc_driver __read_mostly xhci_plat_hc_driver;
-- 
2.8.1

[RESEND PATCH v2 5/7] usb: xhci: plat: Remove checks for optional clock in error/remove path

[Jisheng Zhang]

Commit 63589e92c2d9 ("clk: Ignore error and NULL pointers passed to
clk_{unprepare, disable}()") allows NULL or error pointer to be passed
unconditionally.

This patch is to simplify probe error and remove code paths.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/usb/host/xhci-plat.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 0e69712..83669d0 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -266,8 +266,7 @@ put_usb3_hcd:
 	usb_put_hcd(xhci->shared_hcd);
 
 disable_clk:
-	if (!IS_ERR(clk))
-		clk_disable_unprepare(clk);
+	clk_disable_unprepare(clk);
 
 put_hcd:
 	usb_put_hcd(hcd);
@@ -287,8 +286,7 @@ static int xhci_plat_remove(struct platform_device *dev)
 	usb_remove_hcd(hcd);
 	usb_put_hcd(xhci->shared_hcd);
 
-	if (!IS_ERR(clk))
-		clk_disable_unprepare(clk);
+	clk_disable_unprepare(clk);
 	usb_put_hcd(hcd);
 
 	return 0;
-- 
2.8.1

[RESEND PATCH v2 5/7] usb: xhci: plat: Remove checks for optional clock in error/remove path

[Jisheng Zhang]

Commit 63589e92c2d9 ("clk: Ignore error and NULL pointers passed to
clk_{unprepare, disable}()") allows NULL or error pointer to be passed
unconditionally.

This patch is to simplify probe error and remove code paths.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/usb/host/xhci-plat.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 0e69712..83669d0 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -266,8 +266,7 @@ put_usb3_hcd:
 	usb_put_hcd(xhci->shared_hcd);
 
 disable_clk:
-	if (!IS_ERR(clk))
-		clk_disable_unprepare(clk);
+	clk_disable_unprepare(clk);
 
 put_hcd:
 	usb_put_hcd(hcd);
@@ -287,8 +286,7 @@ static int xhci_plat_remove(struct platform_device *dev)
 	usb_remove_hcd(hcd);
 	usb_put_hcd(xhci->shared_hcd);
 
-	if (!IS_ERR(clk))
-		clk_disable_unprepare(clk);
+	clk_disable_unprepare(clk);
 	usb_put_hcd(hcd);
 
 	return 0;
-- 
2.8.1

[RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Jisheng Zhang]

Marvell BG4CT SoC needs two phy: one for usb2 and another for usb3. Add
the calls to retrieve generic PHY to xhci plat in order to support this.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/usb/host/xhci-plat.c | 87 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 75 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 83669d0..d7f4f3c 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -16,6 +16,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/usb/phy.h>
@@ -134,6 +135,37 @@ static const struct of_device_id usb_xhci_of_match[] = {
 MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
 #endif
 
+static int xhci_plat_phy_init(struct usb_hcd *hcd)
+{
+	int ret;
+
+	if (hcd->phy) {
+		ret = phy_init(hcd->phy);
+		if (ret)
+			return ret;
+
+		ret = phy_power_on(hcd->phy);
+		if (ret) {
+			phy_exit(hcd->phy);
+			return ret;
+		}
+	} else {
+		ret = usb_phy_init(hcd->usb_phy);
+	}
+
+	return ret;
+}
+
+static void xhci_plat_phy_exit(struct usb_hcd *hcd)
+{
+	if (hcd->phy) {
+		phy_power_off(hcd->phy);
+		phy_exit(hcd->phy);
+	} else {
+		usb_phy_shutdown(hcd->usb_phy);
+	}
+}
+
 static int xhci_plat_probe(struct platform_device *pdev)
 {
 	struct device_node	*node = pdev->dev.of_node;
@@ -145,6 +177,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	struct usb_hcd		*hcd;
 	struct clk              *clk;
 	struct usb_phy		*usb_phy;
+	struct phy		*phy;
 	int			ret;
 	int			irq;
 
@@ -232,22 +265,44 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
 		xhci->shared_hcd->can_do_streams = 1;
 
+	hcd->phy = devm_phy_get(&pdev->dev, "usb2-phy");
+	if (IS_ERR(hcd->phy)) {
+		ret = PTR_ERR(hcd->phy);
+		if (ret == -EPROBE_DEFER)
+			goto put_usb3_hcd;
+		hcd->phy = NULL;
+	}
+
+	phy = devm_phy_get(&pdev->dev, "usb-phy");
+	if (IS_ERR(phy)) {
+		ret = PTR_ERR(phy);
+		if (ret == -EPROBE_DEFER)
+			goto put_usb3_hcd;
+		phy = NULL;
+	}
+
 	usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
 	if (IS_ERR(usb_phy)) {
 		ret = PTR_ERR(usb_phy);
 		if (ret == -EPROBE_DEFER)
 			goto put_usb3_hcd;
 		usb_phy = NULL;
-	} else {
-		ret = usb_phy_init(usb_phy);
-		if (ret)
-			goto put_usb3_hcd;
 	}
+
 	xhci->shared_hcd->usb_phy = usb_phy;
+	xhci->shared_hcd->phy = phy;
+
+	ret = xhci_plat_phy_init(hcd);
+	if (ret)
+		goto put_usb3_hcd;
+
+	ret = xhci_plat_phy_init(xhci->shared_hcd);
+	if (ret)
+		goto disable_usb2_phy;
 
 	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (ret)
-		goto disable_usb_phy;
+		goto disable_usb3_phy;
 
 	ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
 	if (ret)
@@ -259,8 +314,11 @@ static int xhci_plat_probe(struct platform_device *pdev)
 dealloc_usb2_hcd:
 	usb_remove_hcd(hcd);
 
-disable_usb_phy:
-	usb_phy_shutdown(usb_phy);
+disable_usb3_phy:
+	xhci_plat_phy_exit(xhci->shared_hcd);
+
+disable_usb2_phy:
+	xhci_plat_phy_exit(hcd);
 
 put_usb3_hcd:
 	usb_put_hcd(xhci->shared_hcd);
@@ -281,11 +339,11 @@ static int xhci_plat_remove(struct platform_device *dev)
 	struct clk *clk = xhci->clk;
 
 	usb_remove_hcd(xhci->shared_hcd);
-	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
-
-	usb_remove_hcd(hcd);
+	xhci_plat_phy_exit(xhci->shared_hcd);
 	usb_put_hcd(xhci->shared_hcd);
 
+	usb_remove_hcd(hcd);
+	xhci_plat_phy_exit(hcd);
 	clk_disable_unprepare(clk);
 	usb_put_hcd(hcd);
 
@@ -311,7 +369,8 @@ static int xhci_plat_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
-	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
+	xhci_plat_phy_exit(xhci->shared_hcd);
+	xhci_plat_phy_exit(hcd);
 	clk_disable_unprepare(xhci->clk);
 
 	return ret;
@@ -327,7 +386,11 @@ static int xhci_plat_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = usb_phy_init(xhci->shared_hcd->usb_phy);
+	ret = xhci_plat_phy_init(hcd);
+	if (ret)
+		return ret;
+
+	ret = xhci_plat_phy_init(xhci->shared_hcd);
 	if (ret)
 		return ret;
 
-- 
2.8.1

[RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Jisheng Zhang]

Marvell BG4CT SoC needs two phy: one for usb2 and another for usb3. Add
the calls to retrieve generic PHY to xhci plat in order to support this.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/usb/host/xhci-plat.c | 87 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 75 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 83669d0..d7f4f3c 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -16,6 +16,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/usb/phy.h>
@@ -134,6 +135,37 @@ static const struct of_device_id usb_xhci_of_match[] = {
 MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
 #endif
 
+static int xhci_plat_phy_init(struct usb_hcd *hcd)
+{
+	int ret;
+
+	if (hcd->phy) {
+		ret = phy_init(hcd->phy);
+		if (ret)
+			return ret;
+
+		ret = phy_power_on(hcd->phy);
+		if (ret) {
+			phy_exit(hcd->phy);
+			return ret;
+		}
+	} else {
+		ret = usb_phy_init(hcd->usb_phy);
+	}
+
+	return ret;
+}
+
+static void xhci_plat_phy_exit(struct usb_hcd *hcd)
+{
+	if (hcd->phy) {
+		phy_power_off(hcd->phy);
+		phy_exit(hcd->phy);
+	} else {
+		usb_phy_shutdown(hcd->usb_phy);
+	}
+}
+
 static int xhci_plat_probe(struct platform_device *pdev)
 {
 	struct device_node	*node = pdev->dev.of_node;
@@ -145,6 +177,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	struct usb_hcd		*hcd;
 	struct clk              *clk;
 	struct usb_phy		*usb_phy;
+	struct phy		*phy;
 	int			ret;
 	int			irq;
 
@@ -232,22 +265,44 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
 		xhci->shared_hcd->can_do_streams = 1;
 
+	hcd->phy = devm_phy_get(&pdev->dev, "usb2-phy");
+	if (IS_ERR(hcd->phy)) {
+		ret = PTR_ERR(hcd->phy);
+		if (ret == -EPROBE_DEFER)
+			goto put_usb3_hcd;
+		hcd->phy = NULL;
+	}
+
+	phy = devm_phy_get(&pdev->dev, "usb-phy");
+	if (IS_ERR(phy)) {
+		ret = PTR_ERR(phy);
+		if (ret == -EPROBE_DEFER)
+			goto put_usb3_hcd;
+		phy = NULL;
+	}
+
 	usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
 	if (IS_ERR(usb_phy)) {
 		ret = PTR_ERR(usb_phy);
 		if (ret == -EPROBE_DEFER)
 			goto put_usb3_hcd;
 		usb_phy = NULL;
-	} else {
-		ret = usb_phy_init(usb_phy);
-		if (ret)
-			goto put_usb3_hcd;
 	}
+
 	xhci->shared_hcd->usb_phy = usb_phy;
+	xhci->shared_hcd->phy = phy;
+
+	ret = xhci_plat_phy_init(hcd);
+	if (ret)
+		goto put_usb3_hcd;
+
+	ret = xhci_plat_phy_init(xhci->shared_hcd);
+	if (ret)
+		goto disable_usb2_phy;
 
 	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (ret)
-		goto disable_usb_phy;
+		goto disable_usb3_phy;
 
 	ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
 	if (ret)
@@ -259,8 +314,11 @@ static int xhci_plat_probe(struct platform_device *pdev)
 dealloc_usb2_hcd:
 	usb_remove_hcd(hcd);
 
-disable_usb_phy:
-	usb_phy_shutdown(usb_phy);
+disable_usb3_phy:
+	xhci_plat_phy_exit(xhci->shared_hcd);
+
+disable_usb2_phy:
+	xhci_plat_phy_exit(hcd);
 
 put_usb3_hcd:
 	usb_put_hcd(xhci->shared_hcd);
@@ -281,11 +339,11 @@ static int xhci_plat_remove(struct platform_device *dev)
 	struct clk *clk = xhci->clk;
 
 	usb_remove_hcd(xhci->shared_hcd);
-	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
-
-	usb_remove_hcd(hcd);
+	xhci_plat_phy_exit(xhci->shared_hcd);
 	usb_put_hcd(xhci->shared_hcd);
 
+	usb_remove_hcd(hcd);
+	xhci_plat_phy_exit(hcd);
 	clk_disable_unprepare(clk);
 	usb_put_hcd(hcd);
 
@@ -311,7 +369,8 @@ static int xhci_plat_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
-	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
+	xhci_plat_phy_exit(xhci->shared_hcd);
+	xhci_plat_phy_exit(hcd);
 	clk_disable_unprepare(xhci->clk);
 
 	return ret;
@@ -327,7 +386,11 @@ static int xhci_plat_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = usb_phy_init(xhci->shared_hcd->usb_phy);
+	ret = xhci_plat_phy_init(hcd);
+	if (ret)
+		return ret;
+
+	ret = xhci_plat_phy_init(xhci->shared_hcd);
 	if (ret)
 		return ret;
 
-- 
2.8.1

[RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Jisheng Zhang]

The Marvell BG4CT STB board has board level vbus control through gpio.
This patch adds the vbus regulator control to support this board.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/usb/host/xhci-plat.c | 40 +++++++++++++++++++++++++++++++++++++++-
 drivers/usb/host/xhci.h      |  2 ++
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index d7f4f3c..0310c13 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -18,6 +18,7 @@
 #include <linux/of.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/usb/phy.h>
 #include <linux/usb/xhci_pdriver.h>
@@ -178,6 +179,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	struct clk              *clk;
 	struct usb_phy		*usb_phy;
 	struct phy		*phy;
+	struct regulator	*vbus;
 	int			ret;
 	int			irq;
 
@@ -249,13 +251,30 @@ static int xhci_plat_probe(struct platform_device *pdev)
 
 	device_wakeup_enable(hcd->self.controller);
 
+	vbus = devm_regulator_get(&pdev->dev, "vbus");
+	if (PTR_ERR(vbus) == -ENODEV) {
+		vbus = NULL;
+	} else if (IS_ERR(vbus)) {
+		ret = PTR_ERR(vbus);
+		goto disable_clk;
+	} else if (vbus) {
+		ret = regulator_enable(vbus);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"failed to enable usb vbus regulator: %d\n",
+				ret);
+			goto disable_clk;
+		}
+	}
+
 	xhci->clk = clk;
+	xhci->vbus = vbus;
 	xhci->main_hcd = hcd;
 	xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev,
 			dev_name(&pdev->dev), hcd);
 	if (!xhci->shared_hcd) {
 		ret = -ENOMEM;
-		goto disable_clk;
+		goto disable_vbus;
 	}
 
 	if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
@@ -323,6 +342,10 @@ disable_usb2_phy:
 put_usb3_hcd:
 	usb_put_hcd(xhci->shared_hcd);
 
+disable_vbus:
+	if (vbus)
+		regulator_disable(vbus);
+
 disable_clk:
 	clk_disable_unprepare(clk);
 
@@ -337,6 +360,7 @@ static int xhci_plat_remove(struct platform_device *dev)
 	struct usb_hcd	*hcd = platform_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 	struct clk *clk = xhci->clk;
+	struct regulator *vbus = xhci->vbus;
 
 	usb_remove_hcd(xhci->shared_hcd);
 	xhci_plat_phy_exit(xhci->shared_hcd);
@@ -347,6 +371,9 @@ static int xhci_plat_remove(struct platform_device *dev)
 	clk_disable_unprepare(clk);
 	usb_put_hcd(hcd);
 
+	if (vbus)
+		regulator_disable(vbus);
+
 	return 0;
 }
 
@@ -356,6 +383,7 @@ static int xhci_plat_suspend(struct device *dev)
 	int ret;
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	struct regulator *vbus = xhci->vbus;
 
 	/*
 	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
@@ -373,6 +401,9 @@ static int xhci_plat_suspend(struct device *dev)
 	xhci_plat_phy_exit(hcd);
 	clk_disable_unprepare(xhci->clk);
 
+	if (vbus)
+		ret = regulator_disable(vbus);
+
 	return ret;
 }
 
@@ -381,11 +412,18 @@ static int xhci_plat_resume(struct device *dev)
 	int ret;
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	struct regulator *vbus = xhci->vbus;
 
 	ret = clk_prepare_enable(xhci->clk);
 	if (ret)
 		return ret;
 
+	if (vbus) {
+		ret = regulator_enable(vbus);
+		if (ret)
+			return ret;
+	}
+
 	ret = xhci_plat_phy_init(hcd);
 	if (ret)
 		return ret;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 6c629c9..5fa8662 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1550,6 +1550,8 @@ struct xhci_hcd {
 	struct msix_entry	*msix_entries;
 	/* optional clock */
 	struct clk		*clk;
+	/* optional regulator */
+	struct regulator	*vbus;
 	/* data structures */
 	struct xhci_device_context_array *dcbaa;
 	struct xhci_ring	*cmd_ring;
-- 
2.8.1

[RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Jisheng Zhang]

The Marvell BG4CT STB board has board level vbus control through gpio.
This patch adds the vbus regulator control to support this board.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/usb/host/xhci-plat.c | 40 +++++++++++++++++++++++++++++++++++++++-
 drivers/usb/host/xhci.h      |  2 ++
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index d7f4f3c..0310c13 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -18,6 +18,7 @@
 #include <linux/of.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/usb/phy.h>
 #include <linux/usb/xhci_pdriver.h>
@@ -178,6 +179,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	struct clk              *clk;
 	struct usb_phy		*usb_phy;
 	struct phy		*phy;
+	struct regulator	*vbus;
 	int			ret;
 	int			irq;
 
@@ -249,13 +251,30 @@ static int xhci_plat_probe(struct platform_device *pdev)
 
 	device_wakeup_enable(hcd->self.controller);
 
+	vbus = devm_regulator_get(&pdev->dev, "vbus");
+	if (PTR_ERR(vbus) == -ENODEV) {
+		vbus = NULL;
+	} else if (IS_ERR(vbus)) {
+		ret = PTR_ERR(vbus);
+		goto disable_clk;
+	} else if (vbus) {
+		ret = regulator_enable(vbus);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"failed to enable usb vbus regulator: %d\n",
+				ret);
+			goto disable_clk;
+		}
+	}
+
 	xhci->clk = clk;
+	xhci->vbus = vbus;
 	xhci->main_hcd = hcd;
 	xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev,
 			dev_name(&pdev->dev), hcd);
 	if (!xhci->shared_hcd) {
 		ret = -ENOMEM;
-		goto disable_clk;
+		goto disable_vbus;
 	}
 
 	if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
@@ -323,6 +342,10 @@ disable_usb2_phy:
 put_usb3_hcd:
 	usb_put_hcd(xhci->shared_hcd);
 
+disable_vbus:
+	if (vbus)
+		regulator_disable(vbus);
+
 disable_clk:
 	clk_disable_unprepare(clk);
 
@@ -337,6 +360,7 @@ static int xhci_plat_remove(struct platform_device *dev)
 	struct usb_hcd	*hcd = platform_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 	struct clk *clk = xhci->clk;
+	struct regulator *vbus = xhci->vbus;
 
 	usb_remove_hcd(xhci->shared_hcd);
 	xhci_plat_phy_exit(xhci->shared_hcd);
@@ -347,6 +371,9 @@ static int xhci_plat_remove(struct platform_device *dev)
 	clk_disable_unprepare(clk);
 	usb_put_hcd(hcd);
 
+	if (vbus)
+		regulator_disable(vbus);
+
 	return 0;
 }
 
@@ -356,6 +383,7 @@ static int xhci_plat_suspend(struct device *dev)
 	int ret;
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	struct regulator *vbus = xhci->vbus;
 
 	/*
 	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
@@ -373,6 +401,9 @@ static int xhci_plat_suspend(struct device *dev)
 	xhci_plat_phy_exit(hcd);
 	clk_disable_unprepare(xhci->clk);
 
+	if (vbus)
+		ret = regulator_disable(vbus);
+
 	return ret;
 }
 
@@ -381,11 +412,18 @@ static int xhci_plat_resume(struct device *dev)
 	int ret;
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	struct regulator *vbus = xhci->vbus;
 
 	ret = clk_prepare_enable(xhci->clk);
 	if (ret)
 		return ret;
 
+	if (vbus) {
+		ret = regulator_enable(vbus);
+		if (ret)
+			return ret;
+	}
+
 	ret = xhci_plat_phy_init(hcd);
 	if (ret)
 		return ret;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 6c629c9..5fa8662 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1550,6 +1550,8 @@ struct xhci_hcd {
 	struct msix_entry	*msix_entries;
 	/* optional clock */
 	struct clk		*clk;
+	/* optional regulator */
+	struct regulator	*vbus;
 	/* data structures */
 	struct xhci_device_context_array *dcbaa;
 	struct xhci_ring	*cmd_ring;
-- 
2.8.1

Re: [RESEND PATCH v2 1/7] usb: xhci: plat: Fix suspend/resume when the optional clk exists

[Felipe Balbi]

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


Hi,

(since you're fixing somebody else's commit, it's nice to Cc authors)

Jisheng Zhang <jszhang@marvell.com> writes:
> Commit 4718c1774051 ("usb: host: xhci-plat: add clock support") adds
> optional clk support, but it forgets to prepare/disable and
                                          ^^^^^^^^^^^^^^^
                                          prepare/enable ?

> enable/unprepare the clk in the resume/suspend path. This path fixes
  ^^^^^^^^^^^^^^^^                                          ^^^^
  disable/unprepare ?                                       patch

> this issue by adding missing clk related calls.

frankly, I'm not sure this patch is entirely correct. At minimum, it's
not necessarily a bug fix. Original commit had no intent in gating
clocks during suspend/resume and, IMHO, that might not be what *all*
XHCI implementations want, though I'm not entirely sure.

> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> Fixes: 4718c1774051 ("usb: host: xhci-plat: add clock support")

Assuming this is, indeed, a fix; you need to Cc stable here. Just add:

Cc: <stable@vger.kernel.org> # v3.16+

> ---
>  drivers/usb/host/xhci-plat.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 474b5fa..8cb46cb 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -295,6 +295,7 @@ static int xhci_plat_remove(struct platform_device *dev)
>  #ifdef CONFIG_PM_SLEEP
>  static int xhci_plat_suspend(struct device *dev)
>  {
> +	int ret;

this would look neater after hcd and xhci declarations below

>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>  
> @@ -306,14 +307,25 @@ static int xhci_plat_suspend(struct device *dev)
>  	 * reconsider this when xhci_plat_suspend enlarges its scope, e.g.,
>  	 * also applies to runtime suspend.
>  	 */
> -	return xhci_suspend(xhci, device_may_wakeup(dev));
> +	ret = xhci_suspend(xhci, device_may_wakeup(dev));
> +	if (ret)
> +		return ret;
> +
> +	clk_disable_unprepare(xhci->clk);
> +
> +	return ret;
>  }
>  
>  static int xhci_plat_resume(struct device *dev)
>  {
> +	int ret;

ditto

>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>  
> +	ret = clk_prepare_enable(xhci->clk);
> +	if (ret)
> +		return ret;
> +
>  	return xhci_resume(xhci, 0);
>  }
>  
> -- 
> 2.8.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[RESEND PATCH v2 1/7] usb: xhci: plat: Fix suspend/resume when the optional clk exists

[Felipe Balbi]

Hi,

(since you're fixing somebody else's commit, it's nice to Cc authors)

Jisheng Zhang <jszhang@marvell.com> writes:
> Commit 4718c1774051 ("usb: host: xhci-plat: add clock support") adds
> optional clk support, but it forgets to prepare/disable and
                                          ^^^^^^^^^^^^^^^
                                          prepare/enable ?

> enable/unprepare the clk in the resume/suspend path. This path fixes
  ^^^^^^^^^^^^^^^^                                          ^^^^
  disable/unprepare ?                                       patch

> this issue by adding missing clk related calls.

frankly, I'm not sure this patch is entirely correct. At minimum, it's
not necessarily a bug fix. Original commit had no intent in gating
clocks during suspend/resume and, IMHO, that might not be what *all*
XHCI implementations want, though I'm not entirely sure.

> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> Fixes: 4718c1774051 ("usb: host: xhci-plat: add clock support")

Assuming this is, indeed, a fix; you need to Cc stable here. Just add:

Cc: <stable@vger.kernel.org> # v3.16+

> ---
>  drivers/usb/host/xhci-plat.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 474b5fa..8cb46cb 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -295,6 +295,7 @@ static int xhci_plat_remove(struct platform_device *dev)
>  #ifdef CONFIG_PM_SLEEP
>  static int xhci_plat_suspend(struct device *dev)
>  {
> +	int ret;

this would look neater after hcd and xhci declarations below

>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>  
> @@ -306,14 +307,25 @@ static int xhci_plat_suspend(struct device *dev)
>  	 * reconsider this when xhci_plat_suspend enlarges its scope, e.g.,
>  	 * also applies to runtime suspend.
>  	 */
> -	return xhci_suspend(xhci, device_may_wakeup(dev));
> +	ret = xhci_suspend(xhci, device_may_wakeup(dev));
> +	if (ret)
> +		return ret;
> +
> +	clk_disable_unprepare(xhci->clk);
> +
> +	return ret;
>  }
>  
>  static int xhci_plat_resume(struct device *dev)
>  {
> +	int ret;

ditto

>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>  
> +	ret = clk_prepare_enable(xhci->clk);
> +	if (ret)
> +		return ret;
> +
>  	return xhci_resume(xhci, 0);
>  }
>  
> -- 
> 2.8.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160427/4c64ec1f/attachment-0001.sig>

Re: [RESEND PATCH v2 2/7] usb: xhci: plat: attach the usb_phy to the correct hcd

[Felipe Balbi]

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


Hi,

(Cc authors and maintainer, otherwise you're patch might be forgotten ;-)

Jisheng Zhang <jszhang@marvell.com> writes:
> Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") adds the
> usb_phy for usb3, but it attached the usb_phy to incorrect hcd. The

where did you see that's the USB3 phy ? I can't see it.

> xhci->shared_hcd is the hcd for usb3, this patch fixes this issue
> by attach the usb_phy to the xhci->shared_hcd.

Fixes: 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support")
Cc: <stable@vger.kernel.org # v4.1+

Maxime, any comments ? It _is_ odd that only one PHY got added.

> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/usb/host/xhci-plat.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 8cb46cb..9ff89e9 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -144,6 +144,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	struct resource         *res;
>  	struct usb_hcd		*hcd;
>  	struct clk              *clk;
> +	struct usb_phy		*usb_phy;
>  	int			ret;
>  	int			irq;
>  
> @@ -231,17 +232,18 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
>  		xhci->shared_hcd->can_do_streams = 1;
>  
> -	hcd->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
> -	if (IS_ERR(hcd->usb_phy)) {
> -		ret = PTR_ERR(hcd->usb_phy);
> +	usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
> +	if (IS_ERR(usb_phy)) {
> +		ret = PTR_ERR(usb_phy);
>  		if (ret == -EPROBE_DEFER)
>  			goto put_usb3_hcd;
> -		hcd->usb_phy = NULL;
> +		usb_phy = NULL;
>  	} else {
> -		ret = usb_phy_init(hcd->usb_phy);
> +		ret = usb_phy_init(usb_phy);
>  		if (ret)
>  			goto put_usb3_hcd;
>  	}
> +	xhci->shared_hcd->usb_phy = usb_phy;
>  
>  	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
>  	if (ret)
> @@ -258,7 +260,7 @@ dealloc_usb2_hcd:
>  	usb_remove_hcd(hcd);
>  
>  disable_usb_phy:
> -	usb_phy_shutdown(hcd->usb_phy);
> +	usb_phy_shutdown(usb_phy);
>  
>  put_usb3_hcd:
>  	usb_put_hcd(xhci->shared_hcd);
> @@ -280,7 +282,7 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	struct clk *clk = xhci->clk;
>  
>  	usb_remove_hcd(xhci->shared_hcd);
> -	usb_phy_shutdown(hcd->usb_phy);
> +	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
>  
>  	usb_remove_hcd(hcd);
>  	usb_put_hcd(xhci->shared_hcd);
> -- 
> 2.8.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[RESEND PATCH v2 2/7] usb: xhci: plat: attach the usb_phy to the correct hcd

[Felipe Balbi]

Hi,

(Cc authors and maintainer, otherwise you're patch might be forgotten ;-)

Jisheng Zhang <jszhang@marvell.com> writes:
> Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") adds the
> usb_phy for usb3, but it attached the usb_phy to incorrect hcd. The

where did you see that's the USB3 phy ? I can't see it.

> xhci->shared_hcd is the hcd for usb3, this patch fixes this issue
> by attach the usb_phy to the xhci->shared_hcd.

Fixes: 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support")
Cc: <stable@vger.kernel.org # v4.1+

Maxime, any comments ? It _is_ odd that only one PHY got added.

> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/usb/host/xhci-plat.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 8cb46cb..9ff89e9 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -144,6 +144,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	struct resource         *res;
>  	struct usb_hcd		*hcd;
>  	struct clk              *clk;
> +	struct usb_phy		*usb_phy;
>  	int			ret;
>  	int			irq;
>  
> @@ -231,17 +232,18 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
>  		xhci->shared_hcd->can_do_streams = 1;
>  
> -	hcd->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
> -	if (IS_ERR(hcd->usb_phy)) {
> -		ret = PTR_ERR(hcd->usb_phy);
> +	usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
> +	if (IS_ERR(usb_phy)) {
> +		ret = PTR_ERR(usb_phy);
>  		if (ret == -EPROBE_DEFER)
>  			goto put_usb3_hcd;
> -		hcd->usb_phy = NULL;
> +		usb_phy = NULL;
>  	} else {
> -		ret = usb_phy_init(hcd->usb_phy);
> +		ret = usb_phy_init(usb_phy);
>  		if (ret)
>  			goto put_usb3_hcd;
>  	}
> +	xhci->shared_hcd->usb_phy = usb_phy;
>  
>  	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
>  	if (ret)
> @@ -258,7 +260,7 @@ dealloc_usb2_hcd:
>  	usb_remove_hcd(hcd);
>  
>  disable_usb_phy:
> -	usb_phy_shutdown(hcd->usb_phy);
> +	usb_phy_shutdown(usb_phy);
>  
>  put_usb3_hcd:
>  	usb_put_hcd(xhci->shared_hcd);
> @@ -280,7 +282,7 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	struct clk *clk = xhci->clk;
>  
>  	usb_remove_hcd(xhci->shared_hcd);
> -	usb_phy_shutdown(hcd->usb_phy);
> +	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
>  
>  	usb_remove_hcd(hcd);
>  	usb_put_hcd(xhci->shared_hcd);
> -- 
> 2.8.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160427/1bb1a626/attachment.sig>

Re: [RESEND PATCH v2 3/7] usb: xhci: plat: Fix suspend/resume when the optional usb_phy exists

[Felipe Balbi]

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


Hi,

Jisheng Zhang <jszhang@marvell.com> writes:
> Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") adds the
> usb_phy for usb3, but it forgets to shutdown/init the usb_phy in the
> suspend/resume path. This patch fixes this issue by adding missing
> usb_phy related calls.

Fixes: 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support")
Cc: <stable@vger.kernel.org> # v4.1+

> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/usb/host/xhci-plat.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 9ff89e9..fbd23fd 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -313,6 +313,7 @@ static int xhci_plat_suspend(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
>  	clk_disable_unprepare(xhci->clk);
>  
>  	return ret;
> @@ -328,6 +329,10 @@ static int xhci_plat_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	ret = usb_phy_init(xhci->shared_hcd->usb_phy);
> +	if (ret)
> +		return ret;
> +
>  	return xhci_resume(xhci, 0);
>  }

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[RESEND PATCH v2 3/7] usb: xhci: plat: Fix suspend/resume when the optional usb_phy exists

[Felipe Balbi]

Hi,

Jisheng Zhang <jszhang@marvell.com> writes:
> Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") adds the
> usb_phy for usb3, but it forgets to shutdown/init the usb_phy in the
> suspend/resume path. This patch fixes this issue by adding missing
> usb_phy related calls.

Fixes: 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support")
Cc: <stable@vger.kernel.org> # v4.1+

> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/usb/host/xhci-plat.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 9ff89e9..fbd23fd 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -313,6 +313,7 @@ static int xhci_plat_suspend(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
>  	clk_disable_unprepare(xhci->clk);
>  
>  	return ret;
> @@ -328,6 +329,10 @@ static int xhci_plat_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	ret = usb_phy_init(xhci->shared_hcd->usb_phy);
> +	if (ret)
> +		return ret;
> +
>  	return xhci_resume(xhci, 0);
>  }

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160427/e713f363/attachment.sig>

Re: [RESEND PATCH v2 5/7] usb: xhci: plat: Remove checks for optional clock in error/remove path

[Felipe Balbi]

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

Jisheng Zhang <jszhang@marvell.com> writes:
> Commit 63589e92c2d9 ("clk: Ignore error and NULL pointers passed to
> clk_{unprepare, disable}()") allows NULL or error pointer to be passed
> unconditionally.
>
> This patch is to simplify probe error and remove code paths.

this seems wrong to me. xhci->clk isn't initialized to NULL, it's either
initialized to a valid struct clk * or some ERR_PTR() value.

Care to explain ?

> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/usb/host/xhci-plat.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 0e69712..83669d0 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -266,8 +266,7 @@ put_usb3_hcd:
>  	usb_put_hcd(xhci->shared_hcd);
>  
>  disable_clk:
> -	if (!IS_ERR(clk))
> -		clk_disable_unprepare(clk);
> +	clk_disable_unprepare(clk);
>  
>  put_hcd:
>  	usb_put_hcd(hcd);
> @@ -287,8 +286,7 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	usb_remove_hcd(hcd);
>  	usb_put_hcd(xhci->shared_hcd);
>  
> -	if (!IS_ERR(clk))
> -		clk_disable_unprepare(clk);
> +	clk_disable_unprepare(clk);
>  	usb_put_hcd(hcd);
>  
>  	return 0;
> -- 
> 2.8.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[RESEND PATCH v2 5/7] usb: xhci: plat: Remove checks for optional clock in error/remove path

[Felipe Balbi]

Jisheng Zhang <jszhang@marvell.com> writes:
> Commit 63589e92c2d9 ("clk: Ignore error and NULL pointers passed to
> clk_{unprepare, disable}()") allows NULL or error pointer to be passed
> unconditionally.
>
> This patch is to simplify probe error and remove code paths.

this seems wrong to me. xhci->clk isn't initialized to NULL, it's either
initialized to a valid struct clk * or some ERR_PTR() value.

Care to explain ?

> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/usb/host/xhci-plat.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 0e69712..83669d0 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -266,8 +266,7 @@ put_usb3_hcd:
>  	usb_put_hcd(xhci->shared_hcd);
>  
>  disable_clk:
> -	if (!IS_ERR(clk))
> -		clk_disable_unprepare(clk);
> +	clk_disable_unprepare(clk);
>  
>  put_hcd:
>  	usb_put_hcd(hcd);
> @@ -287,8 +286,7 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	usb_remove_hcd(hcd);
>  	usb_put_hcd(xhci->shared_hcd);
>  
> -	if (!IS_ERR(clk))
> -		clk_disable_unprepare(clk);
> +	clk_disable_unprepare(clk);
>  	usb_put_hcd(hcd);
>  
>  	return 0;
> -- 
> 2.8.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160427/9eb13fb2/attachment.sig>

Re: [RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Felipe Balbi]

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


Hi,

Jisheng Zhang <jszhang@marvell.com> writes:
> Marvell BG4CT SoC needs two phy: one for usb2 and another for usb3. Add
> the calls to retrieve generic PHY to xhci plat in order to support this.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/usb/host/xhci-plat.c | 87 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 75 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 83669d0..d7f4f3c 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -16,6 +16,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/usb/phy.h>
> @@ -134,6 +135,37 @@ static const struct of_device_id usb_xhci_of_match[] = {
>  MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
>  #endif
>  
> +static int xhci_plat_phy_init(struct usb_hcd *hcd)
> +{
> +	int ret;
> +
> +	if (hcd->phy) {
> +		ret = phy_init(hcd->phy);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_power_on(hcd->phy);
> +		if (ret) {
> +			phy_exit(hcd->phy);
> +			return ret;
> +		}
> +	} else {
> +		ret = usb_phy_init(hcd->usb_phy);
> +	}
> +
> +	return ret;
> +}
> +
> +static void xhci_plat_phy_exit(struct usb_hcd *hcd)
> +{
> +	if (hcd->phy) {
> +		phy_power_off(hcd->phy);
> +		phy_exit(hcd->phy);
> +	} else {
> +		usb_phy_shutdown(hcd->usb_phy);
> +	}
> +}
> +
>  static int xhci_plat_probe(struct platform_device *pdev)
>  {
>  	struct device_node	*node = pdev->dev.of_node;
> @@ -145,6 +177,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	struct usb_hcd		*hcd;
>  	struct clk              *clk;
>  	struct usb_phy		*usb_phy;
> +	struct phy		*phy;

so, one phy driver using USB PHY layer and another using generic PHY
layer ? Why ? I think the first thing your series should do would be to
add proper suport for both APIs with two PHYs and make them all optional
for xhci-plat.

> @@ -232,22 +265,44 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
>  		xhci->shared_hcd->can_do_streams = 1;
>  
> +	hcd->phy = devm_phy_get(&pdev->dev, "usb2-phy");
> +	if (IS_ERR(hcd->phy)) {
> +		ret = PTR_ERR(hcd->phy);
> +		if (ret == -EPROBE_DEFER)
> +			goto put_usb3_hcd;
> +		hcd->phy = NULL;
> +	}
> +
> +	phy = devm_phy_get(&pdev->dev, "usb-phy");
> +	if (IS_ERR(phy)) {
> +		ret = PTR_ERR(phy);
> +		if (ret == -EPROBE_DEFER)
> +			goto put_usb3_hcd;
> +		phy = NULL;
> +	}
> +
>  	usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
>  	if (IS_ERR(usb_phy)) {
>  		ret = PTR_ERR(usb_phy);
>  		if (ret == -EPROBE_DEFER)
>  			goto put_usb3_hcd;
>  		usb_phy = NULL;
> -	} else {
> -		ret = usb_phy_init(usb_phy);
> -		if (ret)
> -			goto put_usb3_hcd;
>  	}
> +
>  	xhci->shared_hcd->usb_phy = usb_phy;
> +	xhci->shared_hcd->phy = phy;
> +
> +	ret = xhci_plat_phy_init(hcd);
> +	if (ret)
> +		goto put_usb3_hcd;
> +
> +	ret = xhci_plat_phy_init(xhci->shared_hcd);
> +	if (ret)
> +		goto disable_usb2_phy;
>  
>  	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
>  	if (ret)
> -		goto disable_usb_phy;
> +		goto disable_usb3_phy;
>  
>  	ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
>  	if (ret)
> @@ -259,8 +314,11 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  dealloc_usb2_hcd:
>  	usb_remove_hcd(hcd);
>  
> -disable_usb_phy:
> -	usb_phy_shutdown(usb_phy);
> +disable_usb3_phy:
> +	xhci_plat_phy_exit(xhci->shared_hcd);
> +
> +disable_usb2_phy:
> +	xhci_plat_phy_exit(hcd);
>  
>  put_usb3_hcd:
>  	usb_put_hcd(xhci->shared_hcd);
> @@ -281,11 +339,11 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	struct clk *clk = xhci->clk;
>  
>  	usb_remove_hcd(xhci->shared_hcd);
> -	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
> -
> -	usb_remove_hcd(hcd);
> +	xhci_plat_phy_exit(xhci->shared_hcd);
>  	usb_put_hcd(xhci->shared_hcd);
>  
> +	usb_remove_hcd(hcd);
> +	xhci_plat_phy_exit(hcd);
>  	clk_disable_unprepare(clk);
>  	usb_put_hcd(hcd);
>  
> @@ -311,7 +369,8 @@ static int xhci_plat_suspend(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
> +	xhci_plat_phy_exit(xhci->shared_hcd);
> +	xhci_plat_phy_exit(hcd);
>  	clk_disable_unprepare(xhci->clk);
>  
>  	return ret;
> @@ -327,7 +386,11 @@ static int xhci_plat_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	ret = usb_phy_init(xhci->shared_hcd->usb_phy);
> +	ret = xhci_plat_phy_init(hcd);
> +	if (ret)
> +		return ret;
> +
> +	ret = xhci_plat_phy_init(xhci->shared_hcd);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.8.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Felipe Balbi]

Hi,

Jisheng Zhang <jszhang@marvell.com> writes:
> Marvell BG4CT SoC needs two phy: one for usb2 and another for usb3. Add
> the calls to retrieve generic PHY to xhci plat in order to support this.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/usb/host/xhci-plat.c | 87 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 75 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 83669d0..d7f4f3c 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -16,6 +16,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/usb/phy.h>
> @@ -134,6 +135,37 @@ static const struct of_device_id usb_xhci_of_match[] = {
>  MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
>  #endif
>  
> +static int xhci_plat_phy_init(struct usb_hcd *hcd)
> +{
> +	int ret;
> +
> +	if (hcd->phy) {
> +		ret = phy_init(hcd->phy);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_power_on(hcd->phy);
> +		if (ret) {
> +			phy_exit(hcd->phy);
> +			return ret;
> +		}
> +	} else {
> +		ret = usb_phy_init(hcd->usb_phy);
> +	}
> +
> +	return ret;
> +}
> +
> +static void xhci_plat_phy_exit(struct usb_hcd *hcd)
> +{
> +	if (hcd->phy) {
> +		phy_power_off(hcd->phy);
> +		phy_exit(hcd->phy);
> +	} else {
> +		usb_phy_shutdown(hcd->usb_phy);
> +	}
> +}
> +
>  static int xhci_plat_probe(struct platform_device *pdev)
>  {
>  	struct device_node	*node = pdev->dev.of_node;
> @@ -145,6 +177,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	struct usb_hcd		*hcd;
>  	struct clk              *clk;
>  	struct usb_phy		*usb_phy;
> +	struct phy		*phy;

so, one phy driver using USB PHY layer and another using generic PHY
layer ? Why ? I think the first thing your series should do would be to
add proper suport for both APIs with two PHYs and make them all optional
for xhci-plat.

> @@ -232,22 +265,44 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
>  		xhci->shared_hcd->can_do_streams = 1;
>  
> +	hcd->phy = devm_phy_get(&pdev->dev, "usb2-phy");
> +	if (IS_ERR(hcd->phy)) {
> +		ret = PTR_ERR(hcd->phy);
> +		if (ret == -EPROBE_DEFER)
> +			goto put_usb3_hcd;
> +		hcd->phy = NULL;
> +	}
> +
> +	phy = devm_phy_get(&pdev->dev, "usb-phy");
> +	if (IS_ERR(phy)) {
> +		ret = PTR_ERR(phy);
> +		if (ret == -EPROBE_DEFER)
> +			goto put_usb3_hcd;
> +		phy = NULL;
> +	}
> +
>  	usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
>  	if (IS_ERR(usb_phy)) {
>  		ret = PTR_ERR(usb_phy);
>  		if (ret == -EPROBE_DEFER)
>  			goto put_usb3_hcd;
>  		usb_phy = NULL;
> -	} else {
> -		ret = usb_phy_init(usb_phy);
> -		if (ret)
> -			goto put_usb3_hcd;
>  	}
> +
>  	xhci->shared_hcd->usb_phy = usb_phy;
> +	xhci->shared_hcd->phy = phy;
> +
> +	ret = xhci_plat_phy_init(hcd);
> +	if (ret)
> +		goto put_usb3_hcd;
> +
> +	ret = xhci_plat_phy_init(xhci->shared_hcd);
> +	if (ret)
> +		goto disable_usb2_phy;
>  
>  	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
>  	if (ret)
> -		goto disable_usb_phy;
> +		goto disable_usb3_phy;
>  
>  	ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
>  	if (ret)
> @@ -259,8 +314,11 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  dealloc_usb2_hcd:
>  	usb_remove_hcd(hcd);
>  
> -disable_usb_phy:
> -	usb_phy_shutdown(usb_phy);
> +disable_usb3_phy:
> +	xhci_plat_phy_exit(xhci->shared_hcd);
> +
> +disable_usb2_phy:
> +	xhci_plat_phy_exit(hcd);
>  
>  put_usb3_hcd:
>  	usb_put_hcd(xhci->shared_hcd);
> @@ -281,11 +339,11 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	struct clk *clk = xhci->clk;
>  
>  	usb_remove_hcd(xhci->shared_hcd);
> -	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
> -
> -	usb_remove_hcd(hcd);
> +	xhci_plat_phy_exit(xhci->shared_hcd);
>  	usb_put_hcd(xhci->shared_hcd);
>  
> +	usb_remove_hcd(hcd);
> +	xhci_plat_phy_exit(hcd);
>  	clk_disable_unprepare(clk);
>  	usb_put_hcd(hcd);
>  
> @@ -311,7 +369,8 @@ static int xhci_plat_suspend(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
> +	xhci_plat_phy_exit(xhci->shared_hcd);
> +	xhci_plat_phy_exit(hcd);
>  	clk_disable_unprepare(xhci->clk);
>  
>  	return ret;
> @@ -327,7 +386,11 @@ static int xhci_plat_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	ret = usb_phy_init(xhci->shared_hcd->usb_phy);
> +	ret = xhci_plat_phy_init(hcd);
> +	if (ret)
> +		return ret;
> +
> +	ret = xhci_plat_phy_init(xhci->shared_hcd);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.8.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160427/dadf98da/attachment.sig>

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Felipe Balbi]

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


Hi,

Jisheng Zhang <jszhang@marvell.com> writes:
> The Marvell BG4CT STB board has board level vbus control through gpio.
> This patch adds the vbus regulator control to support this board.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/usb/host/xhci-plat.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  drivers/usb/host/xhci.h      |  2 ++
>  2 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index d7f4f3c..0310c13 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -18,6 +18,7 @@
>  #include <linux/of.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/usb/phy.h>
>  #include <linux/usb/xhci_pdriver.h>
> @@ -178,6 +179,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	struct clk              *clk;
>  	struct usb_phy		*usb_phy;
>  	struct phy		*phy;
> +	struct regulator	*vbus;
>  	int			ret;
>  	int			irq;
>  
> @@ -249,13 +251,30 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  
>  	device_wakeup_enable(hcd->self.controller);
>  
> +	vbus = devm_regulator_get(&pdev->dev, "vbus");

devm_regulator_get_optional() ??

> +	if (PTR_ERR(vbus) == -ENODEV) {
> +		vbus = NULL;
> +	} else if (IS_ERR(vbus)) {
> +		ret = PTR_ERR(vbus);
> +		goto disable_clk;
> +	} else if (vbus) {
> +		ret = regulator_enable(vbus);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"failed to enable usb vbus regulator: %d\n",
> +				ret);
> +			goto disable_clk;
> +		}
> +	}
> +
>  	xhci->clk = clk;
> +	xhci->vbus = vbus;
>  	xhci->main_hcd = hcd;
>  	xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev,
>  			dev_name(&pdev->dev), hcd);
>  	if (!xhci->shared_hcd) {
>  		ret = -ENOMEM;
> -		goto disable_clk;
> +		goto disable_vbus;
>  	}
>  
>  	if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
> @@ -323,6 +342,10 @@ disable_usb2_phy:
>  put_usb3_hcd:
>  	usb_put_hcd(xhci->shared_hcd);
>  
> +disable_vbus:
> +	if (vbus)
> +		regulator_disable(vbus);
> +
>  disable_clk:
>  	clk_disable_unprepare(clk);
>  
> @@ -337,6 +360,7 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	struct usb_hcd	*hcd = platform_get_drvdata(dev);
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>  	struct clk *clk = xhci->clk;
> +	struct regulator *vbus = xhci->vbus;
>  
>  	usb_remove_hcd(xhci->shared_hcd);
>  	xhci_plat_phy_exit(xhci->shared_hcd);
> @@ -347,6 +371,9 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	clk_disable_unprepare(clk);
>  	usb_put_hcd(hcd);
>  
> +	if (vbus)
> +		regulator_disable(vbus);
> +
>  	return 0;
>  }
>  
> @@ -356,6 +383,7 @@ static int xhci_plat_suspend(struct device *dev)
>  	int ret;
>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +	struct regulator *vbus = xhci->vbus;
>  
>  	/*
>  	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
> @@ -373,6 +401,9 @@ static int xhci_plat_suspend(struct device *dev)
>  	xhci_plat_phy_exit(hcd);
>  	clk_disable_unprepare(xhci->clk);
>  
> +	if (vbus)
> +		ret = regulator_disable(vbus);
> +
>  	return ret;
>  }
>  
> @@ -381,11 +412,18 @@ static int xhci_plat_resume(struct device *dev)
>  	int ret;
>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +	struct regulator *vbus = xhci->vbus;
>  
>  	ret = clk_prepare_enable(xhci->clk);
>  	if (ret)
>  		return ret;
>  
> +	if (vbus) {
> +		ret = regulator_enable(vbus);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = xhci_plat_phy_init(hcd);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 6c629c9..5fa8662 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1550,6 +1550,8 @@ struct xhci_hcd {
>  	struct msix_entry	*msix_entries;
>  	/* optional clock */
>  	struct clk		*clk;
> +	/* optional regulator */
> +	struct regulator	*vbus;
>  	/* data structures */
>  	struct xhci_device_context_array *dcbaa;
>  	struct xhci_ring	*cmd_ring;
> -- 
> 2.8.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Felipe Balbi]

Hi,

Jisheng Zhang <jszhang@marvell.com> writes:
> The Marvell BG4CT STB board has board level vbus control through gpio.
> This patch adds the vbus regulator control to support this board.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/usb/host/xhci-plat.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  drivers/usb/host/xhci.h      |  2 ++
>  2 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index d7f4f3c..0310c13 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -18,6 +18,7 @@
>  #include <linux/of.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/usb/phy.h>
>  #include <linux/usb/xhci_pdriver.h>
> @@ -178,6 +179,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	struct clk              *clk;
>  	struct usb_phy		*usb_phy;
>  	struct phy		*phy;
> +	struct regulator	*vbus;
>  	int			ret;
>  	int			irq;
>  
> @@ -249,13 +251,30 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  
>  	device_wakeup_enable(hcd->self.controller);
>  
> +	vbus = devm_regulator_get(&pdev->dev, "vbus");

devm_regulator_get_optional() ??

> +	if (PTR_ERR(vbus) == -ENODEV) {
> +		vbus = NULL;
> +	} else if (IS_ERR(vbus)) {
> +		ret = PTR_ERR(vbus);
> +		goto disable_clk;
> +	} else if (vbus) {
> +		ret = regulator_enable(vbus);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"failed to enable usb vbus regulator: %d\n",
> +				ret);
> +			goto disable_clk;
> +		}
> +	}
> +
>  	xhci->clk = clk;
> +	xhci->vbus = vbus;
>  	xhci->main_hcd = hcd;
>  	xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev,
>  			dev_name(&pdev->dev), hcd);
>  	if (!xhci->shared_hcd) {
>  		ret = -ENOMEM;
> -		goto disable_clk;
> +		goto disable_vbus;
>  	}
>  
>  	if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
> @@ -323,6 +342,10 @@ disable_usb2_phy:
>  put_usb3_hcd:
>  	usb_put_hcd(xhci->shared_hcd);
>  
> +disable_vbus:
> +	if (vbus)
> +		regulator_disable(vbus);
> +
>  disable_clk:
>  	clk_disable_unprepare(clk);
>  
> @@ -337,6 +360,7 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	struct usb_hcd	*hcd = platform_get_drvdata(dev);
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>  	struct clk *clk = xhci->clk;
> +	struct regulator *vbus = xhci->vbus;
>  
>  	usb_remove_hcd(xhci->shared_hcd);
>  	xhci_plat_phy_exit(xhci->shared_hcd);
> @@ -347,6 +371,9 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	clk_disable_unprepare(clk);
>  	usb_put_hcd(hcd);
>  
> +	if (vbus)
> +		regulator_disable(vbus);
> +
>  	return 0;
>  }
>  
> @@ -356,6 +383,7 @@ static int xhci_plat_suspend(struct device *dev)
>  	int ret;
>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +	struct regulator *vbus = xhci->vbus;
>  
>  	/*
>  	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
> @@ -373,6 +401,9 @@ static int xhci_plat_suspend(struct device *dev)
>  	xhci_plat_phy_exit(hcd);
>  	clk_disable_unprepare(xhci->clk);
>  
> +	if (vbus)
> +		ret = regulator_disable(vbus);
> +
>  	return ret;
>  }
>  
> @@ -381,11 +412,18 @@ static int xhci_plat_resume(struct device *dev)
>  	int ret;
>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +	struct regulator *vbus = xhci->vbus;
>  
>  	ret = clk_prepare_enable(xhci->clk);
>  	if (ret)
>  		return ret;
>  
> +	if (vbus) {
> +		ret = regulator_enable(vbus);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = xhci_plat_phy_init(hcd);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 6c629c9..5fa8662 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1550,6 +1550,8 @@ struct xhci_hcd {
>  	struct msix_entry	*msix_entries;
>  	/* optional clock */
>  	struct clk		*clk;
> +	/* optional regulator */
> +	struct regulator	*vbus;
>  	/* data structures */
>  	struct xhci_device_context_array *dcbaa;
>  	struct xhci_ring	*cmd_ring;
> -- 
> 2.8.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160427/dc562ee1/attachment.sig>

Re: [RESEND PATCH v2 1/7] usb: xhci: plat: Fix suspend/resume when the optional clk exists

[Jisheng Zhang]

Dear Felipe,

On Wed, 27 Apr 2016 08:25:38 +0300 Felipe Balbi wrote:

> Hi,
> 
> (since you're fixing somebody else's commit, it's nice to Cc authors)
> 
> Jisheng Zhang <jszhang@marvell.com> writes:
> > Commit 4718c1774051 ("usb: host: xhci-plat: add clock support") adds
> > optional clk support, but it forgets to prepare/disable and  
>                                           ^^^^^^^^^^^^^^^
>                                           prepare/enable ?
> 
> > enable/unprepare the clk in the resume/suspend path. This path fixes  
>   ^^^^^^^^^^^^^^^^                                          ^^^^
>   disable/unprepare ?                                       patch
> 
> > this issue by adding missing clk related calls.  
> 
> frankly, I'm not sure this patch is entirely correct. At minimum, it's
> not necessarily a bug fix. Original commit had no intent in gating
> clocks during suspend/resume and, IMHO, that might not be what *all*
> XHCI implementations want, though I'm not entirely sure.

Thanks for the hint. Indeed, that's not all xhci-plat users want. I'll drop
this patch in v3.

Thanks a lot for the review,
Jisheng

> 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > Fixes: 4718c1774051 ("usb: host: xhci-plat: add clock support")  
> 
> Assuming this is, indeed, a fix; you need to Cc stable here. Just add:
> 
> Cc: <stable@vger.kernel.org> # v3.16+
> 
> > ---
> >  drivers/usb/host/xhci-plat.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 474b5fa..8cb46cb 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -295,6 +295,7 @@ static int xhci_plat_remove(struct platform_device *dev)
> >  #ifdef CONFIG_PM_SLEEP
> >  static int xhci_plat_suspend(struct device *dev)
> >  {
> > +	int ret;  
> 
> this would look neater after hcd and xhci declarations below
> 
> >  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
> >  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> >  
> > @@ -306,14 +307,25 @@ static int xhci_plat_suspend(struct device *dev)
> >  	 * reconsider this when xhci_plat_suspend enlarges its scope, e.g.,
> >  	 * also applies to runtime suspend.
> >  	 */
> > -	return xhci_suspend(xhci, device_may_wakeup(dev));
> > +	ret = xhci_suspend(xhci, device_may_wakeup(dev));
> > +	if (ret)
> > +		return ret;
> > +
> > +	clk_disable_unprepare(xhci->clk);
> > +
> > +	return ret;
> >  }
> >  
> >  static int xhci_plat_resume(struct device *dev)
> >  {
> > +	int ret;  
> 
> ditto
> 
> >  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
> >  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> >  
> > +	ret = clk_prepare_enable(xhci->clk);
> > +	if (ret)
> > +		return ret;
> > +
> >  	return xhci_resume(xhci, 0);
> >  }
> >  
> > -- 
> > 2.8.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 

[RESEND PATCH v2 1/7] usb: xhci: plat: Fix suspend/resume when the optional clk exists

[Jisheng Zhang]

Dear Felipe,

On Wed, 27 Apr 2016 08:25:38 +0300 Felipe Balbi wrote:

> Hi,
> 
> (since you're fixing somebody else's commit, it's nice to Cc authors)
> 
> Jisheng Zhang <jszhang@marvell.com> writes:
> > Commit 4718c1774051 ("usb: host: xhci-plat: add clock support") adds
> > optional clk support, but it forgets to prepare/disable and  
>                                           ^^^^^^^^^^^^^^^
>                                           prepare/enable ?
> 
> > enable/unprepare the clk in the resume/suspend path. This path fixes  
>   ^^^^^^^^^^^^^^^^                                          ^^^^
>   disable/unprepare ?                                       patch
> 
> > this issue by adding missing clk related calls.  
> 
> frankly, I'm not sure this patch is entirely correct. At minimum, it's
> not necessarily a bug fix. Original commit had no intent in gating
> clocks during suspend/resume and, IMHO, that might not be what *all*
> XHCI implementations want, though I'm not entirely sure.

Thanks for the hint. Indeed, that's not all xhci-plat users want. I'll drop
this patch in v3.

Thanks a lot for the review,
Jisheng

> 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > Fixes: 4718c1774051 ("usb: host: xhci-plat: add clock support")  
> 
> Assuming this is, indeed, a fix; you need to Cc stable here. Just add:
> 
> Cc: <stable@vger.kernel.org> # v3.16+
> 
> > ---
> >  drivers/usb/host/xhci-plat.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 474b5fa..8cb46cb 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -295,6 +295,7 @@ static int xhci_plat_remove(struct platform_device *dev)
> >  #ifdef CONFIG_PM_SLEEP
> >  static int xhci_plat_suspend(struct device *dev)
> >  {
> > +	int ret;  
> 
> this would look neater after hcd and xhci declarations below
> 
> >  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
> >  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> >  
> > @@ -306,14 +307,25 @@ static int xhci_plat_suspend(struct device *dev)
> >  	 * reconsider this when xhci_plat_suspend enlarges its scope, e.g.,
> >  	 * also applies to runtime suspend.
> >  	 */
> > -	return xhci_suspend(xhci, device_may_wakeup(dev));
> > +	ret = xhci_suspend(xhci, device_may_wakeup(dev));
> > +	if (ret)
> > +		return ret;
> > +
> > +	clk_disable_unprepare(xhci->clk);
> > +
> > +	return ret;
> >  }
> >  
> >  static int xhci_plat_resume(struct device *dev)
> >  {
> > +	int ret;  
> 
> ditto
> 
> >  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
> >  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> >  
> > +	ret = clk_prepare_enable(xhci->clk);
> > +	if (ret)
> > +		return ret;
> > +
> >  	return xhci_resume(xhci, 0);
> >  }
> >  
> > -- 
> > 2.8.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 

Re: [RESEND PATCH v2 2/7] usb: xhci: plat: attach the usb_phy to the correct hcd

[Jisheng Zhang]

Dear Felipe,

On Wed, 27 Apr 2016 08:29:34 +0300 Felipe Balbi wrote:

> Hi,
> 
> (Cc authors and maintainer, otherwise you're patch might be forgotten ;-)

Thanks a lot for the kind remind. I just run get_maintainer.pl to get the
To and Cc lists. It seems Mathias's email changed, MAINTAINERS need to updated.

> 
> Jisheng Zhang <jszhang@marvell.com> writes:
> > Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") adds the
> > usb_phy for usb3, but it attached the usb_phy to incorrect hcd. The  
> 
> where did you see that's the USB3 phy ? I can't see it.

Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") says:

"The Marvell Armada 385 AP needs a dumb phy in order to enable the USB3 VBUS."

So I think it means USB3 phy.

Here I have two questions: Per my understanding, usb_phy is deprecated, all
users and new code are encouraged to use generic phy. I think there must
be some special reason commit 7b8ef22ea547 still made use of usb_phy, could
I know what's the reason?

And could the "USB3 VBUS" support be achieved through regulator framework?

Thanks in advance,
Jisheng


> 
> > xhci->shared_hcd is the hcd for usb3, this patch fixes this issue
> > by attach the usb_phy to the xhci->shared_hcd.  
> 
> Fixes: 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support")
> Cc: <stable@vger.kernel.org # v4.1+
> 
> Maxime, any comments ? It _is_ odd that only one PHY got added.
> 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >  drivers/usb/host/xhci-plat.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 8cb46cb..9ff89e9 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -144,6 +144,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	struct resource         *res;
> >  	struct usb_hcd		*hcd;
> >  	struct clk              *clk;
> > +	struct usb_phy		*usb_phy;
> >  	int			ret;
> >  	int			irq;
> >  
> > @@ -231,17 +232,18 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
> >  		xhci->shared_hcd->can_do_streams = 1;
> >  
> > -	hcd->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
> > -	if (IS_ERR(hcd->usb_phy)) {
> > -		ret = PTR_ERR(hcd->usb_phy);
> > +	usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
> > +	if (IS_ERR(usb_phy)) {
> > +		ret = PTR_ERR(usb_phy);
> >  		if (ret == -EPROBE_DEFER)
> >  			goto put_usb3_hcd;
> > -		hcd->usb_phy = NULL;
> > +		usb_phy = NULL;
> >  	} else {
> > -		ret = usb_phy_init(hcd->usb_phy);
> > +		ret = usb_phy_init(usb_phy);
> >  		if (ret)
> >  			goto put_usb3_hcd;
> >  	}
> > +	xhci->shared_hcd->usb_phy = usb_phy;
> >  
> >  	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
> >  	if (ret)
> > @@ -258,7 +260,7 @@ dealloc_usb2_hcd:
> >  	usb_remove_hcd(hcd);
> >  
> >  disable_usb_phy:
> > -	usb_phy_shutdown(hcd->usb_phy);
> > +	usb_phy_shutdown(usb_phy);
> >  
> >  put_usb3_hcd:
> >  	usb_put_hcd(xhci->shared_hcd);
> > @@ -280,7 +282,7 @@ static int xhci_plat_remove(struct platform_device *dev)
> >  	struct clk *clk = xhci->clk;
> >  
> >  	usb_remove_hcd(xhci->shared_hcd);
> > -	usb_phy_shutdown(hcd->usb_phy);
> > +	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
> >  
> >  	usb_remove_hcd(hcd);
> >  	usb_put_hcd(xhci->shared_hcd);
> > -- 
> > 2.8.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 

[RESEND PATCH v2 2/7] usb: xhci: plat: attach the usb_phy to the correct hcd

[Jisheng Zhang]

Dear Felipe,

On Wed, 27 Apr 2016 08:29:34 +0300 Felipe Balbi wrote:

> Hi,
> 
> (Cc authors and maintainer, otherwise you're patch might be forgotten ;-)

Thanks a lot for the kind remind. I just run get_maintainer.pl to get the
To and Cc lists. It seems Mathias's email changed, MAINTAINERS need to updated.

> 
> Jisheng Zhang <jszhang@marvell.com> writes:
> > Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") adds the
> > usb_phy for usb3, but it attached the usb_phy to incorrect hcd. The  
> 
> where did you see that's the USB3 phy ? I can't see it.

Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") says:

"The Marvell Armada 385 AP needs a dumb phy in order to enable the USB3 VBUS."

So I think it means USB3 phy.

Here I have two questions: Per my understanding, usb_phy is deprecated, all
users and new code are encouraged to use generic phy. I think there must
be some special reason commit 7b8ef22ea547 still made use of usb_phy, could
I know what's the reason?

And could the "USB3 VBUS" support be achieved through regulator framework?

Thanks in advance,
Jisheng


> 
> > xhci->shared_hcd is the hcd for usb3, this patch fixes this issue
> > by attach the usb_phy to the xhci->shared_hcd.  
> 
> Fixes: 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support")
> Cc: <stable@vger.kernel.org # v4.1+
> 
> Maxime, any comments ? It _is_ odd that only one PHY got added.
> 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >  drivers/usb/host/xhci-plat.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 8cb46cb..9ff89e9 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -144,6 +144,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	struct resource         *res;
> >  	struct usb_hcd		*hcd;
> >  	struct clk              *clk;
> > +	struct usb_phy		*usb_phy;
> >  	int			ret;
> >  	int			irq;
> >  
> > @@ -231,17 +232,18 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
> >  		xhci->shared_hcd->can_do_streams = 1;
> >  
> > -	hcd->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
> > -	if (IS_ERR(hcd->usb_phy)) {
> > -		ret = PTR_ERR(hcd->usb_phy);
> > +	usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
> > +	if (IS_ERR(usb_phy)) {
> > +		ret = PTR_ERR(usb_phy);
> >  		if (ret == -EPROBE_DEFER)
> >  			goto put_usb3_hcd;
> > -		hcd->usb_phy = NULL;
> > +		usb_phy = NULL;
> >  	} else {
> > -		ret = usb_phy_init(hcd->usb_phy);
> > +		ret = usb_phy_init(usb_phy);
> >  		if (ret)
> >  			goto put_usb3_hcd;
> >  	}
> > +	xhci->shared_hcd->usb_phy = usb_phy;
> >  
> >  	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
> >  	if (ret)
> > @@ -258,7 +260,7 @@ dealloc_usb2_hcd:
> >  	usb_remove_hcd(hcd);
> >  
> >  disable_usb_phy:
> > -	usb_phy_shutdown(hcd->usb_phy);
> > +	usb_phy_shutdown(usb_phy);
> >  
> >  put_usb3_hcd:
> >  	usb_put_hcd(xhci->shared_hcd);
> > @@ -280,7 +282,7 @@ static int xhci_plat_remove(struct platform_device *dev)
> >  	struct clk *clk = xhci->clk;
> >  
> >  	usb_remove_hcd(xhci->shared_hcd);
> > -	usb_phy_shutdown(hcd->usb_phy);
> > +	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
> >  
> >  	usb_remove_hcd(hcd);
> >  	usb_put_hcd(xhci->shared_hcd);
> > -- 
> > 2.8.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 

Re: [RESEND PATCH v2 2/7] usb: xhci: plat: attach the usb_phy to the correct hcd

[Felipe Balbi]

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


Hi,

Jisheng Zhang <jszhang@marvell.com> writes:
> Dear Felipe,
>
> On Wed, 27 Apr 2016 08:29:34 +0300 Felipe Balbi wrote:
>
>> Hi,
>> 
>> (Cc authors and maintainer, otherwise you're patch might be forgotten ;-)
>
> Thanks a lot for the kind remind. I just run get_maintainer.pl to get the
> To and Cc lists. It seems Mathias's email changed, MAINTAINERS need to updated.
>
>> 
>> Jisheng Zhang <jszhang@marvell.com> writes:
>> > Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") adds the
>> > usb_phy for usb3, but it attached the usb_phy to incorrect hcd. The  
>> 
>> where did you see that's the USB3 phy ? I can't see it.
>
> Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") says:
>
> "The Marvell Armada 385 AP needs a dumb phy in order to enable the USB3 VBUS."

That could be a typo. VBUS is defined by the original USB specification
and there is only a single VBUS ping for all speeds ;-)

> So I think it means USB3 phy.

we should ask patch author :-)

> Here I have two questions: Per my understanding, usb_phy is deprecated, all
> users and new code are encouraged to use generic phy. I think there must
> be some special reason commit 7b8ef22ea547 still made use of usb_phy, could
> I know what's the reason?

I don't know, maybe author knows

> And could the "USB3 VBUS" support be achieved through regulator framework?

depends on how the PHY is integrated but, IMO, it should be doable and
is probably preferrable.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[RESEND PATCH v2 2/7] usb: xhci: plat: attach the usb_phy to the correct hcd

[Felipe Balbi]

Hi,

Jisheng Zhang <jszhang@marvell.com> writes:
> Dear Felipe,
>
> On Wed, 27 Apr 2016 08:29:34 +0300 Felipe Balbi wrote:
>
>> Hi,
>> 
>> (Cc authors and maintainer, otherwise you're patch might be forgotten ;-)
>
> Thanks a lot for the kind remind. I just run get_maintainer.pl to get the
> To and Cc lists. It seems Mathias's email changed, MAINTAINERS need to updated.
>
>> 
>> Jisheng Zhang <jszhang@marvell.com> writes:
>> > Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") adds the
>> > usb_phy for usb3, but it attached the usb_phy to incorrect hcd. The  
>> 
>> where did you see that's the USB3 phy ? I can't see it.
>
> Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") says:
>
> "The Marvell Armada 385 AP needs a dumb phy in order to enable the USB3 VBUS."

That could be a typo. VBUS is defined by the original USB specification
and there is only a single VBUS ping for all speeds ;-)

> So I think it means USB3 phy.

we should ask patch author :-)

> Here I have two questions: Per my understanding, usb_phy is deprecated, all
> users and new code are encouraged to use generic phy. I think there must
> be some special reason commit 7b8ef22ea547 still made use of usb_phy, could
> I know what's the reason?

I don't know, maybe author knows

> And could the "USB3 VBUS" support be achieved through regulator framework?

depends on how the PHY is integrated but, IMO, it should be doable and
is probably preferrable.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160427/4ccee2e6/attachment.sig>

Re: [RESEND PATCH v2 5/7] usb: xhci: plat: Remove checks for optional clock in error/remove path

[Jisheng Zhang]

Dear Felipe,

On Wed, 27 Apr 2016 08:33:52 +0300 Felipe Balbi wrote:

> Jisheng Zhang <jszhang@marvell.com> writes:
> > Commit 63589e92c2d9 ("clk: Ignore error and NULL pointers passed to
> > clk_{unprepare, disable}()") allows NULL or error pointer to be passed
> > unconditionally.
> >
> > This patch is to simplify probe error and remove code paths.  
> 
> this seems wrong to me. xhci->clk isn't initialized to NULL, it's either
> initialized to a valid struct clk * or some ERR_PTR() value.

Commit 63589e92c2d9 could also ignore error value ;)

> 
> Care to explain ?
> 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >  drivers/usb/host/xhci-plat.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 0e69712..83669d0 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -266,8 +266,7 @@ put_usb3_hcd:
> >  	usb_put_hcd(xhci->shared_hcd);
> >  
> >  disable_clk:
> > -	if (!IS_ERR(clk))
> > -		clk_disable_unprepare(clk);
> > +	clk_disable_unprepare(clk);
> >  
> >  put_hcd:
> >  	usb_put_hcd(hcd);
> > @@ -287,8 +286,7 @@ static int xhci_plat_remove(struct platform_device *dev)
> >  	usb_remove_hcd(hcd);
> >  	usb_put_hcd(xhci->shared_hcd);
> >  
> > -	if (!IS_ERR(clk))
> > -		clk_disable_unprepare(clk);
> > +	clk_disable_unprepare(clk);
> >  	usb_put_hcd(hcd);
> >  
> >  	return 0;
> > -- 
> > 2.8.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 

[RESEND PATCH v2 5/7] usb: xhci: plat: Remove checks for optional clock in error/remove path

[Jisheng Zhang]

Dear Felipe,

On Wed, 27 Apr 2016 08:33:52 +0300 Felipe Balbi wrote:

> Jisheng Zhang <jszhang@marvell.com> writes:
> > Commit 63589e92c2d9 ("clk: Ignore error and NULL pointers passed to
> > clk_{unprepare, disable}()") allows NULL or error pointer to be passed
> > unconditionally.
> >
> > This patch is to simplify probe error and remove code paths.  
> 
> this seems wrong to me. xhci->clk isn't initialized to NULL, it's either
> initialized to a valid struct clk * or some ERR_PTR() value.

Commit 63589e92c2d9 could also ignore error value ;)

> 
> Care to explain ?
> 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >  drivers/usb/host/xhci-plat.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 0e69712..83669d0 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -266,8 +266,7 @@ put_usb3_hcd:
> >  	usb_put_hcd(xhci->shared_hcd);
> >  
> >  disable_clk:
> > -	if (!IS_ERR(clk))
> > -		clk_disable_unprepare(clk);
> > +	clk_disable_unprepare(clk);
> >  
> >  put_hcd:
> >  	usb_put_hcd(hcd);
> > @@ -287,8 +286,7 @@ static int xhci_plat_remove(struct platform_device *dev)
> >  	usb_remove_hcd(hcd);
> >  	usb_put_hcd(xhci->shared_hcd);
> >  
> > -	if (!IS_ERR(clk))
> > -		clk_disable_unprepare(clk);
> > +	clk_disable_unprepare(clk);
> >  	usb_put_hcd(hcd);
> >  
> >  	return 0;
> > -- 
> > 2.8.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 

Re: [RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Jisheng Zhang]

Dear Felipe,

On Wed, 27 Apr 2016 08:35:58 +0300 Felipe Balbi wrote:

> Hi,
> 
> Jisheng Zhang <jszhang@marvell.com> writes:
> > Marvell BG4CT SoC needs two phy: one for usb2 and another for usb3. Add
> > the calls to retrieve generic PHY to xhci plat in order to support this.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >  drivers/usb/host/xhci-plat.c | 87 ++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 75 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 83669d0..d7f4f3c 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/dma-mapping.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/phy/phy.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/usb/phy.h>
> > @@ -134,6 +135,37 @@ static const struct of_device_id usb_xhci_of_match[] = {
> >  MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
> >  #endif
> >  
> > +static int xhci_plat_phy_init(struct usb_hcd *hcd)
> > +{
> > +	int ret;
> > +
> > +	if (hcd->phy) {
> > +		ret = phy_init(hcd->phy);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = phy_power_on(hcd->phy);
> > +		if (ret) {
> > +			phy_exit(hcd->phy);
> > +			return ret;
> > +		}
> > +	} else {
> > +		ret = usb_phy_init(hcd->usb_phy);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void xhci_plat_phy_exit(struct usb_hcd *hcd)
> > +{
> > +	if (hcd->phy) {
> > +		phy_power_off(hcd->phy);
> > +		phy_exit(hcd->phy);
> > +	} else {
> > +		usb_phy_shutdown(hcd->usb_phy);
> > +	}
> > +}
> > +
> >  static int xhci_plat_probe(struct platform_device *pdev)
> >  {
> >  	struct device_node	*node = pdev->dev.of_node;
> > @@ -145,6 +177,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	struct usb_hcd		*hcd;
> >  	struct clk              *clk;
> >  	struct usb_phy		*usb_phy;
> > +	struct phy		*phy;  
> 
> so, one phy driver using USB PHY layer and another using generic PHY
> layer ? Why ? I think the first thing your series should do would be to

It's different platforms. E.g
platform A may write the phy driver under usb phy layer, while platform B
may have generic phy driver.

The questions are: when adding phy support to xhci-plat, the generic phy
has existed for a long time, what's the reason to use the deprecated usb
phy APIs.

And per my check, it's only MVEBU platforms use this support? I'm not sure
if we could remove usbphy code from xhci-plat first then add generic phy then
adding MVEBU xhci phy support bak with the new code. So Cc mvebu maintainers

Thanks,
Jisheng

> add proper suport for both APIs with two PHYs and make them all optional
> for xhci-plat.
> 
> > @@ -232,22 +265,44 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
> >  		xhci->shared_hcd->can_do_streams = 1;
> >  
> > +	hcd->phy = devm_phy_get(&pdev->dev, "usb2-phy");
> > +	if (IS_ERR(hcd->phy)) {
> > +		ret = PTR_ERR(hcd->phy);
> > +		if (ret == -EPROBE_DEFER)
> > +			goto put_usb3_hcd;
> > +		hcd->phy = NULL;
> > +	}
> > +
> > +	phy = devm_phy_get(&pdev->dev, "usb-phy");
> > +	if (IS_ERR(phy)) {
> > +		ret = PTR_ERR(phy);
> > +		if (ret == -EPROBE_DEFER)
> > +			goto put_usb3_hcd;
> > +		phy = NULL;
> > +	}
> > +
> >  	usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
> >  	if (IS_ERR(usb_phy)) {
> >  		ret = PTR_ERR(usb_phy);
> >  		if (ret == -EPROBE_DEFER)
> >  			goto put_usb3_hcd;
> >  		usb_phy = NULL;
> > -	} else {
> > -		ret = usb_phy_init(usb_phy);
> > -		if (ret)
> > -			goto put_usb3_hcd;
> >  	}
> > +
> >  	xhci->shared_hcd->usb_phy = usb_phy;
> > +	xhci->shared_hcd->phy = phy;
> > +
> > +	ret = xhci_plat_phy_init(hcd);
> > +	if (ret)
> > +		goto put_usb3_hcd;
> > +
> > +	ret = xhci_plat_phy_init(xhci->shared_hcd);
> > +	if (ret)
> > +		goto disable_usb2_phy;
> >  
> >  	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
> >  	if (ret)
> > -		goto disable_usb_phy;
> > +		goto disable_usb3_phy;
> >  
> >  	ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
> >  	if (ret)
> > @@ -259,8 +314,11 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  dealloc_usb2_hcd:
> >  	usb_remove_hcd(hcd);
> >  
> > -disable_usb_phy:
> > -	usb_phy_shutdown(usb_phy);
> > +disable_usb3_phy:
> > +	xhci_plat_phy_exit(xhci->shared_hcd);
> > +
> > +disable_usb2_phy:
> > +	xhci_plat_phy_exit(hcd);
> >  
> >  put_usb3_hcd:
> >  	usb_put_hcd(xhci->shared_hcd);
> > @@ -281,11 +339,11 @@ static int xhci_plat_remove(struct platform_device *dev)
> >  	struct clk *clk = xhci->clk;
> >  
> >  	usb_remove_hcd(xhci->shared_hcd);
> > -	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
> > -
> > -	usb_remove_hcd(hcd);
> > +	xhci_plat_phy_exit(xhci->shared_hcd);
> >  	usb_put_hcd(xhci->shared_hcd);
> >  
> > +	usb_remove_hcd(hcd);
> > +	xhci_plat_phy_exit(hcd);
> >  	clk_disable_unprepare(clk);
> >  	usb_put_hcd(hcd);
> >  
> > @@ -311,7 +369,8 @@ static int xhci_plat_suspend(struct device *dev)
> >  	if (ret)
> >  		return ret;
> >  
> > -	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
> > +	xhci_plat_phy_exit(xhci->shared_hcd);
> > +	xhci_plat_phy_exit(hcd);
> >  	clk_disable_unprepare(xhci->clk);
> >  
> >  	return ret;
> > @@ -327,7 +386,11 @@ static int xhci_plat_resume(struct device *dev)
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = usb_phy_init(xhci->shared_hcd->usb_phy);
> > +	ret = xhci_plat_phy_init(hcd);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = xhci_plat_phy_init(xhci->shared_hcd);
> >  	if (ret)
> >  		return ret;
> >  
> > -- 
> > 2.8.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 

[RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Jisheng Zhang]

Dear Felipe,

On Wed, 27 Apr 2016 08:35:58 +0300 Felipe Balbi wrote:

> Hi,
> 
> Jisheng Zhang <jszhang@marvell.com> writes:
> > Marvell BG4CT SoC needs two phy: one for usb2 and another for usb3. Add
> > the calls to retrieve generic PHY to xhci plat in order to support this.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >  drivers/usb/host/xhci-plat.c | 87 ++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 75 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 83669d0..d7f4f3c 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/dma-mapping.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/phy/phy.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/usb/phy.h>
> > @@ -134,6 +135,37 @@ static const struct of_device_id usb_xhci_of_match[] = {
> >  MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
> >  #endif
> >  
> > +static int xhci_plat_phy_init(struct usb_hcd *hcd)
> > +{
> > +	int ret;
> > +
> > +	if (hcd->phy) {
> > +		ret = phy_init(hcd->phy);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = phy_power_on(hcd->phy);
> > +		if (ret) {
> > +			phy_exit(hcd->phy);
> > +			return ret;
> > +		}
> > +	} else {
> > +		ret = usb_phy_init(hcd->usb_phy);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void xhci_plat_phy_exit(struct usb_hcd *hcd)
> > +{
> > +	if (hcd->phy) {
> > +		phy_power_off(hcd->phy);
> > +		phy_exit(hcd->phy);
> > +	} else {
> > +		usb_phy_shutdown(hcd->usb_phy);
> > +	}
> > +}
> > +
> >  static int xhci_plat_probe(struct platform_device *pdev)
> >  {
> >  	struct device_node	*node = pdev->dev.of_node;
> > @@ -145,6 +177,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	struct usb_hcd		*hcd;
> >  	struct clk              *clk;
> >  	struct usb_phy		*usb_phy;
> > +	struct phy		*phy;  
> 
> so, one phy driver using USB PHY layer and another using generic PHY
> layer ? Why ? I think the first thing your series should do would be to

It's different platforms. E.g
platform A may write the phy driver under usb phy layer, while platform B
may have generic phy driver.

The questions are: when adding phy support to xhci-plat, the generic phy
has existed for a long time, what's the reason to use the deprecated usb
phy APIs.

And per my check, it's only MVEBU platforms use this support? I'm not sure
if we could remove usbphy code from xhci-plat first then add generic phy then
adding MVEBU xhci phy support bak with the new code. So Cc mvebu maintainers

Thanks,
Jisheng

> add proper suport for both APIs with two PHYs and make them all optional
> for xhci-plat.
> 
> > @@ -232,22 +265,44 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
> >  		xhci->shared_hcd->can_do_streams = 1;
> >  
> > +	hcd->phy = devm_phy_get(&pdev->dev, "usb2-phy");
> > +	if (IS_ERR(hcd->phy)) {
> > +		ret = PTR_ERR(hcd->phy);
> > +		if (ret == -EPROBE_DEFER)
> > +			goto put_usb3_hcd;
> > +		hcd->phy = NULL;
> > +	}
> > +
> > +	phy = devm_phy_get(&pdev->dev, "usb-phy");
> > +	if (IS_ERR(phy)) {
> > +		ret = PTR_ERR(phy);
> > +		if (ret == -EPROBE_DEFER)
> > +			goto put_usb3_hcd;
> > +		phy = NULL;
> > +	}
> > +
> >  	usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
> >  	if (IS_ERR(usb_phy)) {
> >  		ret = PTR_ERR(usb_phy);
> >  		if (ret == -EPROBE_DEFER)
> >  			goto put_usb3_hcd;
> >  		usb_phy = NULL;
> > -	} else {
> > -		ret = usb_phy_init(usb_phy);
> > -		if (ret)
> > -			goto put_usb3_hcd;
> >  	}
> > +
> >  	xhci->shared_hcd->usb_phy = usb_phy;
> > +	xhci->shared_hcd->phy = phy;
> > +
> > +	ret = xhci_plat_phy_init(hcd);
> > +	if (ret)
> > +		goto put_usb3_hcd;
> > +
> > +	ret = xhci_plat_phy_init(xhci->shared_hcd);
> > +	if (ret)
> > +		goto disable_usb2_phy;
> >  
> >  	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
> >  	if (ret)
> > -		goto disable_usb_phy;
> > +		goto disable_usb3_phy;
> >  
> >  	ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
> >  	if (ret)
> > @@ -259,8 +314,11 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  dealloc_usb2_hcd:
> >  	usb_remove_hcd(hcd);
> >  
> > -disable_usb_phy:
> > -	usb_phy_shutdown(usb_phy);
> > +disable_usb3_phy:
> > +	xhci_plat_phy_exit(xhci->shared_hcd);
> > +
> > +disable_usb2_phy:
> > +	xhci_plat_phy_exit(hcd);
> >  
> >  put_usb3_hcd:
> >  	usb_put_hcd(xhci->shared_hcd);
> > @@ -281,11 +339,11 @@ static int xhci_plat_remove(struct platform_device *dev)
> >  	struct clk *clk = xhci->clk;
> >  
> >  	usb_remove_hcd(xhci->shared_hcd);
> > -	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
> > -
> > -	usb_remove_hcd(hcd);
> > +	xhci_plat_phy_exit(xhci->shared_hcd);
> >  	usb_put_hcd(xhci->shared_hcd);
> >  
> > +	usb_remove_hcd(hcd);
> > +	xhci_plat_phy_exit(hcd);
> >  	clk_disable_unprepare(clk);
> >  	usb_put_hcd(hcd);
> >  
> > @@ -311,7 +369,8 @@ static int xhci_plat_suspend(struct device *dev)
> >  	if (ret)
> >  		return ret;
> >  
> > -	usb_phy_shutdown(xhci->shared_hcd->usb_phy);
> > +	xhci_plat_phy_exit(xhci->shared_hcd);
> > +	xhci_plat_phy_exit(hcd);
> >  	clk_disable_unprepare(xhci->clk);
> >  
> >  	return ret;
> > @@ -327,7 +386,11 @@ static int xhci_plat_resume(struct device *dev)
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = usb_phy_init(xhci->shared_hcd->usb_phy);
> > +	ret = xhci_plat_phy_init(hcd);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = xhci_plat_phy_init(xhci->shared_hcd);
> >  	if (ret)
> >  		return ret;
> >  
> > -- 
> > 2.8.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 

Re: [RESEND PATCH v2 5/7] usb: xhci: plat: Remove checks for optional clock in error/remove path

[Felipe Balbi]

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


Hi,

Jisheng Zhang <jszhang@marvell.com> writes:
> Dear Felipe,
>
> On Wed, 27 Apr 2016 08:33:52 +0300 Felipe Balbi wrote:
>
>> Jisheng Zhang <jszhang@marvell.com> writes:
>> > Commit 63589e92c2d9 ("clk: Ignore error and NULL pointers passed to
>> > clk_{unprepare, disable}()") allows NULL or error pointer to be passed
>> > unconditionally.
>> >
>> > This patch is to simplify probe error and remove code paths.  
>> 
>> this seems wrong to me. xhci->clk isn't initialized to NULL, it's either
>> initialized to a valid struct clk * or some ERR_PTR() value.
>
> Commit 63589e92c2d9 could also ignore error value ;)

oh okay, thanks for that. That's, IMHO, quite dangerous ;-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[RESEND PATCH v2 5/7] usb: xhci: plat: Remove checks for optional clock in error/remove path

[Felipe Balbi]

Hi,

Jisheng Zhang <jszhang@marvell.com> writes:
> Dear Felipe,
>
> On Wed, 27 Apr 2016 08:33:52 +0300 Felipe Balbi wrote:
>
>> Jisheng Zhang <jszhang@marvell.com> writes:
>> > Commit 63589e92c2d9 ("clk: Ignore error and NULL pointers passed to
>> > clk_{unprepare, disable}()") allows NULL or error pointer to be passed
>> > unconditionally.
>> >
>> > This patch is to simplify probe error and remove code paths.  
>> 
>> this seems wrong to me. xhci->clk isn't initialized to NULL, it's either
>> initialized to a valid struct clk * or some ERR_PTR() value.
>
> Commit 63589e92c2d9 could also ignore error value ;)

oh okay, thanks for that. That's, IMHO, quite dangerous ;-)

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160427/c7a01efe/attachment.sig>

Re: [RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Felipe Balbi]

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


Hi,

Jisheng Zhang <jszhang@marvell.com> writes:
>> > +static void xhci_plat_phy_exit(struct usb_hcd *hcd)
>> > +{
>> > +	if (hcd->phy) {
>> > +		phy_power_off(hcd->phy);
>> > +		phy_exit(hcd->phy);
>> > +	} else {
>> > +		usb_phy_shutdown(hcd->usb_phy);
>> > +	}
>> > +}
>> > +
>> >  static int xhci_plat_probe(struct platform_device *pdev)
>> >  {
>> >  	struct device_node	*node = pdev->dev.of_node;
>> > @@ -145,6 +177,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>> >  	struct usb_hcd		*hcd;
>> >  	struct clk              *clk;
>> >  	struct usb_phy		*usb_phy;
>> > +	struct phy		*phy;  
>> 
>> so, one phy driver using USB PHY layer and another using generic PHY
>> layer ? Why ? I think the first thing your series should do would be to
>
> It's different platforms. E.g
> platform A may write the phy driver under usb phy layer, while platform B
> may have generic phy driver.

right, but both APIs should be supported with *two* PHYs for the time being.

> The questions are: when adding phy support to xhci-plat, the generic phy
> has existed for a long time, what's the reason to use the deprecated usb
> phy APIs.

I don't know, ask the author :-) Maybe the PHY driver was already
available on the USB PHY layer ? What we should do is push that PHY
driver to be moved over to generic PHY layer, then we can get rid of USB
PHY layer from xhci-plat.

> And per my check, it's only MVEBU platforms use this support? I'm not sure
> if we could remove usbphy code from xhci-plat first then add generic phy then
> adding MVEBU xhci phy support bak with the new code. So Cc mvebu maintainers

First the PHY driver(s) depending on that should be converted over.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Felipe Balbi]

Hi,

Jisheng Zhang <jszhang@marvell.com> writes:
>> > +static void xhci_plat_phy_exit(struct usb_hcd *hcd)
>> > +{
>> > +	if (hcd->phy) {
>> > +		phy_power_off(hcd->phy);
>> > +		phy_exit(hcd->phy);
>> > +	} else {
>> > +		usb_phy_shutdown(hcd->usb_phy);
>> > +	}
>> > +}
>> > +
>> >  static int xhci_plat_probe(struct platform_device *pdev)
>> >  {
>> >  	struct device_node	*node = pdev->dev.of_node;
>> > @@ -145,6 +177,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>> >  	struct usb_hcd		*hcd;
>> >  	struct clk              *clk;
>> >  	struct usb_phy		*usb_phy;
>> > +	struct phy		*phy;  
>> 
>> so, one phy driver using USB PHY layer and another using generic PHY
>> layer ? Why ? I think the first thing your series should do would be to
>
> It's different platforms. E.g
> platform A may write the phy driver under usb phy layer, while platform B
> may have generic phy driver.

right, but both APIs should be supported with *two* PHYs for the time being.

> The questions are: when adding phy support to xhci-plat, the generic phy
> has existed for a long time, what's the reason to use the deprecated usb
> phy APIs.

I don't know, ask the author :-) Maybe the PHY driver was already
available on the USB PHY layer ? What we should do is push that PHY
driver to be moved over to generic PHY layer, then we can get rid of USB
PHY layer from xhci-plat.

> And per my check, it's only MVEBU platforms use this support? I'm not sure
> if we could remove usbphy code from xhci-plat first then add generic phy then
> adding MVEBU xhci phy support bak with the new code. So Cc mvebu maintainers

First the PHY driver(s) depending on that should be converted over.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160427/554d3060/attachment.sig>

Re: [RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Heikki Krogerus]

Hi,

On Tue, Apr 26, 2016 at 08:57:39PM +0800, Jisheng Zhang wrote:
> @@ -232,22 +265,44 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
>  		xhci->shared_hcd->can_do_streams = 1;
>  
> +	hcd->phy = devm_phy_get(&pdev->dev, "usb2-phy");
> +	if (IS_ERR(hcd->phy)) {
> +		ret = PTR_ERR(hcd->phy);
> +		if (ret == -EPROBE_DEFER)
> +			goto put_usb3_hcd;
> +		hcd->phy = NULL;
> +	}
> +
> +	phy = devm_phy_get(&pdev->dev, "usb-phy");

"usb-phy" for what I understand is the USB3 PHY right?

I was unable to find any definition for the phy names for example from
Documentation/devicetree/bindings/usb/usb-xhci.txt, so I would say
this needs to be "usb3-phy" and the phy names need to be defined
somewhere.


Thanks,

-- 
heikki

[RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Heikki Krogerus]

Hi,

On Tue, Apr 26, 2016 at 08:57:39PM +0800, Jisheng Zhang wrote:
> @@ -232,22 +265,44 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
>  		xhci->shared_hcd->can_do_streams = 1;
>  
> +	hcd->phy = devm_phy_get(&pdev->dev, "usb2-phy");
> +	if (IS_ERR(hcd->phy)) {
> +		ret = PTR_ERR(hcd->phy);
> +		if (ret == -EPROBE_DEFER)
> +			goto put_usb3_hcd;
> +		hcd->phy = NULL;
> +	}
> +
> +	phy = devm_phy_get(&pdev->dev, "usb-phy");

"usb-phy" for what I understand is the USB3 PHY right?

I was unable to find any definition for the phy names for example from
Documentation/devicetree/bindings/usb/usb-xhci.txt, so I would say
this needs to be "usb3-phy" and the phy names need to be defined
somewhere.


Thanks,

-- 
heikki

Re: [RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Felipe Balbi]

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


Hi,

Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:
> On Tue, Apr 26, 2016 at 08:57:39PM +0800, Jisheng Zhang wrote:
>> @@ -232,22 +265,44 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>  	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
>>  		xhci->shared_hcd->can_do_streams = 1;
>>  
>> +	hcd->phy = devm_phy_get(&pdev->dev, "usb2-phy");
>> +	if (IS_ERR(hcd->phy)) {
>> +		ret = PTR_ERR(hcd->phy);
>> +		if (ret == -EPROBE_DEFER)
>> +			goto put_usb3_hcd;
>> +		hcd->phy = NULL;
>> +	}
>> +
>> +	phy = devm_phy_get(&pdev->dev, "usb-phy");
>
> "usb-phy" for what I understand is the USB3 PHY right?

that doesn't appear to be documented anywhere ;-)

> I was unable to find any definition for the phy names for example from
> Documentation/devicetree/bindings/usb/usb-xhci.txt, so I would say
> this needs to be "usb3-phy" and the phy names need to be defined
> somewhere.

yeah, usb3-phy sounds like a better name.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Felipe Balbi]

Hi,

Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:
> On Tue, Apr 26, 2016 at 08:57:39PM +0800, Jisheng Zhang wrote:
>> @@ -232,22 +265,44 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>  	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
>>  		xhci->shared_hcd->can_do_streams = 1;
>>  
>> +	hcd->phy = devm_phy_get(&pdev->dev, "usb2-phy");
>> +	if (IS_ERR(hcd->phy)) {
>> +		ret = PTR_ERR(hcd->phy);
>> +		if (ret == -EPROBE_DEFER)
>> +			goto put_usb3_hcd;
>> +		hcd->phy = NULL;
>> +	}
>> +
>> +	phy = devm_phy_get(&pdev->dev, "usb-phy");
>
> "usb-phy" for what I understand is the USB3 PHY right?

that doesn't appear to be documented anywhere ;-)

> I was unable to find any definition for the phy names for example from
> Documentation/devicetree/bindings/usb/usb-xhci.txt, so I would say
> this needs to be "usb3-phy" and the phy names need to be defined
> somewhere.

yeah, usb3-phy sounds like a better name.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160427/4946e7f5/attachment.sig>

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Mark Brown]

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

On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:
> Jisheng Zhang <jszhang@marvell.com> writes:

> > +	vbus = devm_regulator_get(&pdev->dev, "vbus");

> devm_regulator_get_optional() ??

Does USB work without a VBUS?  Unless the answer is yes then I'd expect
this to be just a normal regulator_get().

> 
> > +	if (PTR_ERR(vbus) == -ENODEV) {
> > +		vbus = NULL;
> > +	} else if (IS_ERR(vbus)) {
> > +		ret = PTR_ERR(vbus);
> > +		goto disable_clk;
> > +	} else if (vbus) {
> > +		ret = regulator_enable(vbus);
> > +		if (ret) {
> > +			dev_err(&pdev->dev,
> > +				"failed to enable usb vbus regulator: %d\n",
> > +				ret);
> > +			goto disable_clk;
> > +		}
> > +	}

This is all completely broken unless the supply is optional.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Mark Brown]

On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:
> Jisheng Zhang <jszhang@marvell.com> writes:

> > +	vbus = devm_regulator_get(&pdev->dev, "vbus");

> devm_regulator_get_optional() ??

Does USB work without a VBUS?  Unless the answer is yes then I'd expect
this to be just a normal regulator_get().

> 
> > +	if (PTR_ERR(vbus) == -ENODEV) {
> > +		vbus = NULL;
> > +	} else if (IS_ERR(vbus)) {
> > +		ret = PTR_ERR(vbus);
> > +		goto disable_clk;
> > +	} else if (vbus) {
> > +		ret = regulator_enable(vbus);
> > +		if (ret) {
> > +			dev_err(&pdev->dev,
> > +				"failed to enable usb vbus regulator: %d\n",
> > +				ret);
> > +			goto disable_clk;
> > +		}
> > +	}

This is all completely broken unless the supply is optional.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160427/48c0861d/attachment.sig>

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Jisheng Zhang]

Dear Mark,

On Wed, 27 Apr 2016 10:57:39 +0100 Mark Brown wrote:

> On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:
> > Jisheng Zhang <jszhang@marvell.com> writes:  
> 
> > > +	vbus = devm_regulator_get(&pdev->dev, "vbus");  
> 
> > devm_regulator_get_optional() ??  
> 
> Does USB work without a VBUS?  Unless the answer is yes then I'd expect
> this to be just a normal regulator_get().

Per spec no. But the vbus may be transparent to SW on some platforms, so I
think devm_regulator_get_optional() is better.

> 
> >   
> > > +	if (PTR_ERR(vbus) == -ENODEV) {
> > > +		vbus = NULL;
> > > +	} else if (IS_ERR(vbus)) {
> > > +		ret = PTR_ERR(vbus);
> > > +		goto disable_clk;
> > > +	} else if (vbus) {
> > > +		ret = regulator_enable(vbus);
> > > +		if (ret) {
> > > +			dev_err(&pdev->dev,
> > > +				"failed to enable usb vbus regulator: %d\n",
> > > +				ret);
> > > +			goto disable_clk;
> > > +		}
> > > +	}  
> 
> This is all completely broken unless the supply is optional.

The supply is optional.

Thanks for your review,
Jisheng

[RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Jisheng Zhang]

Dear Mark,

On Wed, 27 Apr 2016 10:57:39 +0100 Mark Brown wrote:

> On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:
> > Jisheng Zhang <jszhang@marvell.com> writes:  
> 
> > > +	vbus = devm_regulator_get(&pdev->dev, "vbus");  
> 
> > devm_regulator_get_optional() ??  
> 
> Does USB work without a VBUS?  Unless the answer is yes then I'd expect
> this to be just a normal regulator_get().

Per spec no. But the vbus may be transparent to SW on some platforms, so I
think devm_regulator_get_optional() is better.

> 
> >   
> > > +	if (PTR_ERR(vbus) == -ENODEV) {
> > > +		vbus = NULL;
> > > +	} else if (IS_ERR(vbus)) {
> > > +		ret = PTR_ERR(vbus);
> > > +		goto disable_clk;
> > > +	} else if (vbus) {
> > > +		ret = regulator_enable(vbus);
> > > +		if (ret) {
> > > +			dev_err(&pdev->dev,
> > > +				"failed to enable usb vbus regulator: %d\n",
> > > +				ret);
> > > +			goto disable_clk;
> > > +		}
> > > +	}  
> 
> This is all completely broken unless the supply is optional.

The supply is optional.

Thanks for your review,
Jisheng

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Felipe Balbi]

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


Hi,

Mark Brown <broonie@kernel.org> writes:
> On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:
>> Jisheng Zhang <jszhang@marvell.com> writes:
>
>> > +	vbus = devm_regulator_get(&pdev->dev, "vbus");
>
>> devm_regulator_get_optional() ??
>
> Does USB work without a VBUS?  Unless the answer is yes then I'd expect

<joke> it can, just source VBUS from a lab power supply </joke>

> this to be just a normal regulator_get().

jokes aside, this regulator is optional because not all platforms
require a SW controlled regulator, no ? Will normal regulator_get() give
us a dummy regulator in case it's not listed in DT/ACPI ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Felipe Balbi]

Hi,

Mark Brown <broonie@kernel.org> writes:
> On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:
>> Jisheng Zhang <jszhang@marvell.com> writes:
>
>> > +	vbus = devm_regulator_get(&pdev->dev, "vbus");
>
>> devm_regulator_get_optional() ??
>
> Does USB work without a VBUS?  Unless the answer is yes then I'd expect

<joke> it can, just source VBUS from a lab power supply </joke>

> this to be just a normal regulator_get().

jokes aside, this regulator is optional because not all platforms
require a SW controlled regulator, no ? Will normal regulator_get() give
us a dummy regulator in case it's not listed in DT/ACPI ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160427/9958c49d/attachment.sig>

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Mark Brown]

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

On Wed, Apr 27, 2016 at 01:25:27PM +0300, Felipe Balbi wrote:
> Mark Brown <broonie@kernel.org> writes:

> > this to be just a normal regulator_get().

> jokes aside, this regulator is optional because not all platforms
> require a SW controlled regulator, no ? Will normal regulator_get() give
> us a dummy regulator in case it's not listed in DT/ACPI ?

Yes we do that, but even regulators that are not software controlled
should really be described anyway since it's a much simpler rule for
people to understand, it ensures that we can just scale up on systems
where there does happen to be software control and it makes all the
resulting code much simpler and hence less error prone if we're not
randomly ignoring some errors.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Mark Brown]

On Wed, Apr 27, 2016 at 01:25:27PM +0300, Felipe Balbi wrote:
> Mark Brown <broonie@kernel.org> writes:

> > this to be just a normal regulator_get().

> jokes aside, this regulator is optional because not all platforms
> require a SW controlled regulator, no ? Will normal regulator_get() give
> us a dummy regulator in case it's not listed in DT/ACPI ?

Yes we do that, but even regulators that are not software controlled
should really be described anyway since it's a much simpler rule for
people to understand, it ensures that we can just scale up on systems
where there does happen to be software control and it makes all the
resulting code much simpler and hence less error prone if we're not
randomly ignoring some errors.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160427/7754733e/attachment.sig>

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Felipe Balbi]

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


Hi,

Mark Brown <broonie@kernel.org> writes:
> On Wed, Apr 27, 2016 at 01:25:27PM +0300, Felipe Balbi wrote:
>> Mark Brown <broonie@kernel.org> writes:
>
>> > this to be just a normal regulator_get().
>
>> jokes aside, this regulator is optional because not all platforms
>> require a SW controlled regulator, no ? Will normal regulator_get() give
>> us a dummy regulator in case it's not listed in DT/ACPI ?
>
> Yes we do that, but even regulators that are not software controlled

okay, good.

> should really be described anyway since it's a much simpler rule for

okay, we'll wait until all vendors update their ACPI tables ;-)

> people to understand, it ensures that we can just scale up on systems
> where there does happen to be software control and it makes all the
> resulting code much simpler and hence less error prone if we're not
> randomly ignoring some errors.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Felipe Balbi]

Hi,

Mark Brown <broonie@kernel.org> writes:
> On Wed, Apr 27, 2016 at 01:25:27PM +0300, Felipe Balbi wrote:
>> Mark Brown <broonie@kernel.org> writes:
>
>> > this to be just a normal regulator_get().
>
>> jokes aside, this regulator is optional because not all platforms
>> require a SW controlled regulator, no ? Will normal regulator_get() give
>> us a dummy regulator in case it's not listed in DT/ACPI ?
>
> Yes we do that, but even regulators that are not software controlled

okay, good.

> should really be described anyway since it's a much simpler rule for

okay, we'll wait until all vendors update their ACPI tables ;-)

> people to understand, it ensures that we can just scale up on systems
> where there does happen to be software control and it makes all the
> resulting code much simpler and hence less error prone if we're not
> randomly ignoring some errors.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160427/f96ae4f0/attachment.sig>

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Mark Brown]

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

On Wed, Apr 27, 2016 at 06:25:16PM +0800, Jisheng Zhang wrote:
> On Wed, 27 Apr 2016 10:57:39 +0100 Mark Brown wrote:
> > On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:

> > > > +	vbus = devm_regulator_get(&pdev->dev, "vbus");  

> > > devm_regulator_get_optional() ??  

> > Does USB work without a VBUS?  Unless the answer is yes then I'd expect
> > this to be just a normal regulator_get().

> Per spec no. But the vbus may be transparent to SW on some platforms, so I
> think devm_regulator_get_optional() is better.

Really, no.  If it's physically there the software needs to be written
as such.  Please see the documentation and list archives for extensive
discussion on this topic.

> > This is all completely broken unless the supply is optional.

> The supply is optional.

To repeat a supply is only optional if it might be physically absent.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Mark Brown]

On Wed, Apr 27, 2016 at 06:25:16PM +0800, Jisheng Zhang wrote:
> On Wed, 27 Apr 2016 10:57:39 +0100 Mark Brown wrote:
> > On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:

> > > > +	vbus = devm_regulator_get(&pdev->dev, "vbus");  

> > > devm_regulator_get_optional() ??  

> > Does USB work without a VBUS?  Unless the answer is yes then I'd expect
> > this to be just a normal regulator_get().

> Per spec no. But the vbus may be transparent to SW on some platforms, so I
> think devm_regulator_get_optional() is better.

Really, no.  If it's physically there the software needs to be written
as such.  Please see the documentation and list archives for extensive
discussion on this topic.

> > This is all completely broken unless the supply is optional.

> The supply is optional.

To repeat a supply is only optional if it might be physically absent.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160427/5c7d5c8b/attachment.sig>