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
Sorting the headers in alphabetic order will help to reduce the conflict
when adding new headers later.
Signed-off-by: Jisheng Zhang <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Hi,
(since you're fixing somebody else's commit, it's nice to Cc authors)
Jisheng Zhang <[email protected]> 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 <[email protected]>
> Fixes: 4718c1774051 ("usb: host: xhci-plat: add clock support")
Assuming this is, indeed, a fix; you need to Cc stable here. Just add:
Cc: <[email protected]> # 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
balbi
Hi,
(Cc authors and maintainer, otherwise you're patch might be forgotten ;-)
Jisheng Zhang <[email protected]> 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: <[email protected] # v4.1+
Maxime, any comments ? It _is_ odd that only one PHY got added.
> Signed-off-by: Jisheng Zhang <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
balbi
Hi,
Jisheng Zhang <[email protected]> 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: <[email protected]> # v4.1+
> Signed-off-by: Jisheng Zhang <[email protected]>
> ---
> 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
Jisheng Zhang <[email protected]> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
balbi
Hi,
Jisheng Zhang <[email protected]> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
balbi
Hi,
Jisheng Zhang <[email protected]> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
balbi
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 <[email protected]> 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 <[email protected]>
> > Fixes: 4718c1774051 ("usb: host: xhci-plat: add clock support")
>
> Assuming this is, indeed, a fix; you need to Cc stable here. Just add:
>
> Cc: <[email protected]> # 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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 <[email protected]> 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: <[email protected] # v4.1+
>
> Maxime, any comments ? It _is_ odd that only one PHY got added.
>
> > Signed-off-by: Jisheng Zhang <[email protected]>
> > ---
> > 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Hi,
Jisheng Zhang <[email protected]> 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 <[email protected]> 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
Dear Felipe,
On Wed, 27 Apr 2016 08:33:52 +0300 Felipe Balbi wrote:
> Jisheng Zhang <[email protected]> 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 <[email protected]>
> > ---
> > 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Dear Felipe,
On Wed, 27 Apr 2016 08:35:58 +0300 Felipe Balbi wrote:
> Hi,
>
> Jisheng Zhang <[email protected]> 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 <[email protected]>
> > ---
> > 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Hi,
Jisheng Zhang <[email protected]> writes:
> Dear Felipe,
>
> On Wed, 27 Apr 2016 08:33:52 +0300 Felipe Balbi wrote:
>
>> Jisheng Zhang <[email protected]> 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
Hi,
Jisheng Zhang <[email protected]> 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
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
Hi,
Heikki Krogerus <[email protected]> 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
On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:
> Jisheng Zhang <[email protected]> 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.
Hi,
Mark Brown <[email protected]> writes:
> On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:
>> Jisheng Zhang <[email protected]> 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
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 <[email protected]> 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
On Wed, Apr 27, 2016 at 01:25:27PM +0300, Felipe Balbi wrote:
> Mark Brown <[email protected]> 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.
Hi,
Mark Brown <[email protected]> writes:
> On Wed, Apr 27, 2016 at 01:25:27PM +0300, Felipe Balbi wrote:
>> Mark Brown <[email protected]> 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
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.