[RFC usb-next v3 0/2] fix HCD PHY suspend handling

[RFC usb-next v3 0/2] fix HCD PHY suspend handling

[Martin Blumenstingl]

This is a follow-up to my previous series "initialize (multiple) PHYs
for a HCD": [0].

Roger Quadros reported [1] that it "is breaking low power cases on TI
SoCs when USB is in host mode". He further explains that "Not doing the
phy_exit() here [when entering suspend] leaves the clocks enabled on
our SoC and we're no longer able to reach low power states on system
suspend."
Chunfeng Yun from Mediatek noted [2] that we cannot unconditionally call
phy_exit while entering system suspend, because this would "disconnect
plugged devices on MTK platforms, due to re-initialize u2 phys when
resume"

In the discussion (which followed Roger's bug report: [1]) Roger,
Chunfeng and me came to the conclusion that we can fix suspend on the
TI SoCs without breaking it on the Mediatek SoCs by extending the
suspend and resume code in usb/core/phy.c by checking whether the USB
controller can wake up the system (which is the case for the Mediatek
MTU3 controller, but now for the dwc3 controller used on the TI SoCs):
- if the controller can wake up the system (Mediatek MTU3 use-case) we
  only call usb_phy_roothub_power_off (which calls phy_power_off) when
  entering system suspend
- if the controller however cannot wake up the system (dwc3 on TI SoCs)
  we additionally call usb_phy_roothub_exit (which calls phy_exit) when
  entering system suspend
- (we undo the previous steps during system resume)

The goal of this series is to fix the issue reported by Roger without
breaking suspend/resume on the Mediatek SoCs.
Since I neither have a TI nor a Mediatek device I am sending this as
RFC. I have tested it on an Amlogic Meson GXM board (Khadas VIM2) which
does NOT support suspend/resume yet.

this should be applied on top of [3] "usb: core: phy: fix return value
of usb_phy_roothub_exit()" (even though there's no strict dependency,
this is the order I wrote the patches in).


changes since RFC v2 at [5]:
- add missing INIT_LIST_HEAD call in usb_phy_roothub_add_phy (affects
  patch #1 - spotted by Roger Quadros, thank you!)
- fixed swapped conditions using device_may_wakeup() in
  usb_phy_roothub_resume because we need to call usb_phy_roothub_init
  if the controller cannot wake up the device (affects patch #2, spotted
  by Chunfeng Yun, thank you!)
- simplified the error condition to "undo" usb_phy_roothub_init if
  usb_phy_roothub_power_on failed in usb_phy_roothub_resume (suggested
  by Chunfeng Yun)
- updated the commit message (using Roger's wording) because (quote from
  Roger "it doesn't prevent the system from entering suspend but just
  prevents the system from reaching lowest power levels in the suspend
  state."

Changes since RFC v1 (blob attachments) at [4]:
- use device_may_wakeup instead of device_can_wakeup as suggested by
  Roger Quadros
- use the controller device from hcd->self.controller as suggested by
  Chunfeng Yun
- compile time fixes thanks to Roger Quadros
- if usb_phy_roothub_power_on in usb_phy_roothub_resume failes then
  we now call usb_phy_roothub_exit to keep the PHYs in the correct
  state if usb_phy_roothub_resume partially failed


[0] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006599.html
[1] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006737.html
[2] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006758.html
[3] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006819.html
[4] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006794.html
[5] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006820.html

Martin Blumenstingl (2):
  usb: core: split usb_phy_roothub_{init,alloc}
  usb: core: use phy_exit during suspend if wake up is not supported

 drivers/usb/core/hcd.c | 18 +++++++----
 drivers/usb/core/phy.c | 88 +++++++++++++++++++++++++++++++++++---------------
 drivers/usb/core/phy.h |  9 +++++-
 3 files changed, 82 insertions(+), 33 deletions(-)

-- 
2.16.3

[RFC usb-next v3 0/2] fix HCD PHY suspend handling

[Martin Blumenstingl]

This is a follow-up to my previous series "initialize (multiple) PHYs
for a HCD": [0].

Roger Quadros reported [1] that it "is breaking low power cases on TI
SoCs when USB is in host mode". He further explains that "Not doing the
phy_exit() here [when entering suspend] leaves the clocks enabled on
our SoC and we're no longer able to reach low power states on system
suspend."
Chunfeng Yun from Mediatek noted [2] that we cannot unconditionally call
phy_exit while entering system suspend, because this would "disconnect
plugged devices on MTK platforms, due to re-initialize u2 phys when
resume"

In the discussion (which followed Roger's bug report: [1]) Roger,
Chunfeng and me came to the conclusion that we can fix suspend on the
TI SoCs without breaking it on the Mediatek SoCs by extending the
suspend and resume code in usb/core/phy.c by checking whether the USB
controller can wake up the system (which is the case for the Mediatek
MTU3 controller, but now for the dwc3 controller used on the TI SoCs):
- if the controller can wake up the system (Mediatek MTU3 use-case) we
  only call usb_phy_roothub_power_off (which calls phy_power_off) when
  entering system suspend
- if the controller however cannot wake up the system (dwc3 on TI SoCs)
  we additionally call usb_phy_roothub_exit (which calls phy_exit) when
  entering system suspend
- (we undo the previous steps during system resume)

The goal of this series is to fix the issue reported by Roger without
breaking suspend/resume on the Mediatek SoCs.
Since I neither have a TI nor a Mediatek device I am sending this as
RFC. I have tested it on an Amlogic Meson GXM board (Khadas VIM2) which
does NOT support suspend/resume yet.

this should be applied on top of [3] "usb: core: phy: fix return value
of usb_phy_roothub_exit()" (even though there's no strict dependency,
this is the order I wrote the patches in).


changes since RFC v2 at [5]:
- add missing INIT_LIST_HEAD call in usb_phy_roothub_add_phy (affects
  patch #1 - spotted by Roger Quadros, thank you!)
- fixed swapped conditions using device_may_wakeup() in
  usb_phy_roothub_resume because we need to call usb_phy_roothub_init
  if the controller cannot wake up the device (affects patch #2, spotted
  by Chunfeng Yun, thank you!)
- simplified the error condition to "undo" usb_phy_roothub_init if
  usb_phy_roothub_power_on failed in usb_phy_roothub_resume (suggested
  by Chunfeng Yun)
- updated the commit message (using Roger's wording) because (quote from
  Roger "it doesn't prevent the system from entering suspend but just
  prevents the system from reaching lowest power levels in the suspend
  state."

Changes since RFC v1 (blob attachments) at [4]:
- use device_may_wakeup instead of device_can_wakeup as suggested by
  Roger Quadros
- use the controller device from hcd->self.controller as suggested by
  Chunfeng Yun
- compile time fixes thanks to Roger Quadros
- if usb_phy_roothub_power_on in usb_phy_roothub_resume failes then
  we now call usb_phy_roothub_exit to keep the PHYs in the correct
  state if usb_phy_roothub_resume partially failed


[0] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006599.html
[1] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006737.html
[2] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006758.html
[3] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006819.html
[4] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006794.html
[5] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006820.html

Martin Blumenstingl (2):
  usb: core: split usb_phy_roothub_{init,alloc}
  usb: core: use phy_exit during suspend if wake up is not supported

 drivers/usb/core/hcd.c | 18 +++++++----
 drivers/usb/core/phy.c | 88 +++++++++++++++++++++++++++++++++++---------------
 drivers/usb/core/phy.h |  9 +++++-
 3 files changed, 82 insertions(+), 33 deletions(-)

-- 
2.16.3

[RFC,usb-next,v3,1/2] usb: core: split usb_phy_roothub_{init,alloc}

[Martin Blumenstingl]

Before this patch usb_phy_roothub_init served two purposes (from a
caller's point of view - like hcd.c):
- parsing the PHYs and allocating the list entries
- calling phy_init on each list entry

While this worked so far it has one disadvantage: if we need to call
phy_init for each PHY instance then the existing code cannot be re-used.
Solve this by splitting off usb_phy_roothub_alloc which only parses the
PHYs and allocates the list entries.
usb_phy_roothub_init then gets a struct usb_phy_roothub and only calls
phy_init on each PHY instance (along with the corresponding cleanup if
that failed somewhere).

This is a preparation step for adding proper suspend support for some
hardware that requires phy_exit to be called during suspend and phy_init
to be called during resume.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/usb/core/hcd.c | 10 +++++++---
 drivers/usb/core/phy.c | 53 +++++++++++++++++++++++++-------------------------
 drivers/usb/core/phy.h |  4 +++-
 3 files changed, 37 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae6367..15b0418e3b6a 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2758,12 +2758,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	}
 
 	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
-		hcd->phy_roothub = usb_phy_roothub_init(hcd->self.sysdev);
+		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
 		if (IS_ERR(hcd->phy_roothub)) {
 			retval = PTR_ERR(hcd->phy_roothub);
-			goto err_phy_roothub_init;
+			goto err_phy_roothub_alloc;
 		}
 
+		retval = usb_phy_roothub_init(hcd->phy_roothub);
+		if (retval)
+			goto err_phy_roothub_alloc;
+
 		retval = usb_phy_roothub_power_on(hcd->phy_roothub);
 		if (retval)
 			goto err_usb_phy_roothub_power_on;
@@ -2936,7 +2940,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	usb_phy_roothub_power_off(hcd->phy_roothub);
 err_usb_phy_roothub_power_on:
 	usb_phy_roothub_exit(hcd->phy_roothub);
-err_phy_roothub_init:
+err_phy_roothub_alloc:
 	if (hcd->remove_phy && hcd->usb_phy) {
 		usb_phy_shutdown(hcd->usb_phy);
 		usb_put_phy(hcd->usb_phy);
diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
index f19aaa3c899c..44f008cda7a8 100644
--- a/drivers/usb/core/phy.c
+++ b/drivers/usb/core/phy.c
@@ -19,19 +19,6 @@ struct usb_phy_roothub {
 	struct list_head	list;
 };
 
-static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
-{
-	struct usb_phy_roothub *roothub_entry;
-
-	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
-	if (!roothub_entry)
-		return ERR_PTR(-ENOMEM);
-
-	INIT_LIST_HEAD(&roothub_entry->list);
-
-	return roothub_entry;
-}
-
 static int usb_phy_roothub_add_phy(struct device *dev, int index,
 				   struct list_head *list)
 {
@@ -45,9 +32,11 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
 			return PTR_ERR(phy);
 	}
 
-	roothub_entry = usb_phy_roothub_alloc(dev);
-	if (IS_ERR(roothub_entry))
-		return PTR_ERR(roothub_entry);
+	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
+	if (!roothub_entry)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&roothub_entry->list);
 
 	roothub_entry->phy = phy;
 
@@ -56,11 +45,9 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
 	return 0;
 }
 
-struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
+struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
 {
 	struct usb_phy_roothub *phy_roothub;
-	struct usb_phy_roothub *roothub_entry;
-	struct list_head *head;
 	int i, num_phys, err;
 
 	num_phys = of_count_phandle_with_args(dev->of_node, "phys",
@@ -68,16 +55,31 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
 	if (num_phys <= 0)
 		return NULL;
 
-	phy_roothub = usb_phy_roothub_alloc(dev);
-	if (IS_ERR(phy_roothub))
-		return phy_roothub;
+	phy_roothub = devm_kzalloc(dev, sizeof(*phy_roothub), GFP_KERNEL);
+	if (!phy_roothub)
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&phy_roothub->list);
 
 	for (i = 0; i < num_phys; i++) {
 		err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list);
 		if (err)
-			goto err_out;
+			return ERR_PTR(err);
 	}
 
+	return phy_roothub;
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_alloc);
+
+int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub)
+{
+	struct usb_phy_roothub *roothub_entry;
+	struct list_head *head;
+	int err;
+
+	if (!phy_roothub)
+		return 0;
+
 	head = &phy_roothub->list;
 
 	list_for_each_entry(roothub_entry, head, list) {
@@ -86,14 +88,13 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
 			goto err_exit_phys;
 	}
 
-	return phy_roothub;
+	return 0;
 
 err_exit_phys:
 	list_for_each_entry_continue_reverse(roothub_entry, head, list)
 		phy_exit(roothub_entry->phy);
 
-err_out:
-	return ERR_PTR(err);
+	return err;
 }
 EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
 
diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
index 6fde59bfbff8..eb31253201ad 100644
--- a/drivers/usb/core/phy.h
+++ b/drivers/usb/core/phy.h
@@ -1,6 +1,8 @@
 struct usb_phy_roothub;
 
-struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev);
+struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);
+
+int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub);
 int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
 
 int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);

[RFC usb-next v3 1/2] usb: core: split usb_phy_roothub_{init,alloc}

[Martin Blumenstingl]

Before this patch usb_phy_roothub_init served two purposes (from a
caller's point of view - like hcd.c):
- parsing the PHYs and allocating the list entries
- calling phy_init on each list entry

While this worked so far it has one disadvantage: if we need to call
phy_init for each PHY instance then the existing code cannot be re-used.
Solve this by splitting off usb_phy_roothub_alloc which only parses the
PHYs and allocates the list entries.
usb_phy_roothub_init then gets a struct usb_phy_roothub and only calls
phy_init on each PHY instance (along with the corresponding cleanup if
that failed somewhere).

This is a preparation step for adding proper suspend support for some
hardware that requires phy_exit to be called during suspend and phy_init
to be called during resume.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 drivers/usb/core/hcd.c | 10 +++++++---
 drivers/usb/core/phy.c | 53 +++++++++++++++++++++++++-------------------------
 drivers/usb/core/phy.h |  4 +++-
 3 files changed, 37 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae6367..15b0418e3b6a 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2758,12 +2758,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	}
 
 	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
-		hcd->phy_roothub = usb_phy_roothub_init(hcd->self.sysdev);
+		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
 		if (IS_ERR(hcd->phy_roothub)) {
 			retval = PTR_ERR(hcd->phy_roothub);
-			goto err_phy_roothub_init;
+			goto err_phy_roothub_alloc;
 		}
 
+		retval = usb_phy_roothub_init(hcd->phy_roothub);
+		if (retval)
+			goto err_phy_roothub_alloc;
+
 		retval = usb_phy_roothub_power_on(hcd->phy_roothub);
 		if (retval)
 			goto err_usb_phy_roothub_power_on;
@@ -2936,7 +2940,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	usb_phy_roothub_power_off(hcd->phy_roothub);
 err_usb_phy_roothub_power_on:
 	usb_phy_roothub_exit(hcd->phy_roothub);
-err_phy_roothub_init:
+err_phy_roothub_alloc:
 	if (hcd->remove_phy && hcd->usb_phy) {
 		usb_phy_shutdown(hcd->usb_phy);
 		usb_put_phy(hcd->usb_phy);
diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
index f19aaa3c899c..44f008cda7a8 100644
--- a/drivers/usb/core/phy.c
+++ b/drivers/usb/core/phy.c
@@ -19,19 +19,6 @@ struct usb_phy_roothub {
 	struct list_head	list;
 };
 
-static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
-{
-	struct usb_phy_roothub *roothub_entry;
-
-	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
-	if (!roothub_entry)
-		return ERR_PTR(-ENOMEM);
-
-	INIT_LIST_HEAD(&roothub_entry->list);
-
-	return roothub_entry;
-}
-
 static int usb_phy_roothub_add_phy(struct device *dev, int index,
 				   struct list_head *list)
 {
@@ -45,9 +32,11 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
 			return PTR_ERR(phy);
 	}
 
-	roothub_entry = usb_phy_roothub_alloc(dev);
-	if (IS_ERR(roothub_entry))
-		return PTR_ERR(roothub_entry);
+	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
+	if (!roothub_entry)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&roothub_entry->list);
 
 	roothub_entry->phy = phy;
 
@@ -56,11 +45,9 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
 	return 0;
 }
 
-struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
+struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
 {
 	struct usb_phy_roothub *phy_roothub;
-	struct usb_phy_roothub *roothub_entry;
-	struct list_head *head;
 	int i, num_phys, err;
 
 	num_phys = of_count_phandle_with_args(dev->of_node, "phys",
@@ -68,16 +55,31 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
 	if (num_phys <= 0)
 		return NULL;
 
-	phy_roothub = usb_phy_roothub_alloc(dev);
-	if (IS_ERR(phy_roothub))
-		return phy_roothub;
+	phy_roothub = devm_kzalloc(dev, sizeof(*phy_roothub), GFP_KERNEL);
+	if (!phy_roothub)
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&phy_roothub->list);
 
 	for (i = 0; i < num_phys; i++) {
 		err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list);
 		if (err)
-			goto err_out;
+			return ERR_PTR(err);
 	}
 
+	return phy_roothub;
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_alloc);
+
+int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub)
+{
+	struct usb_phy_roothub *roothub_entry;
+	struct list_head *head;
+	int err;
+
+	if (!phy_roothub)
+		return 0;
+
 	head = &phy_roothub->list;
 
 	list_for_each_entry(roothub_entry, head, list) {
@@ -86,14 +88,13 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
 			goto err_exit_phys;
 	}
 
-	return phy_roothub;
+	return 0;
 
 err_exit_phys:
 	list_for_each_entry_continue_reverse(roothub_entry, head, list)
 		phy_exit(roothub_entry->phy);
 
-err_out:
-	return ERR_PTR(err);
+	return err;
 }
 EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
 
diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
index 6fde59bfbff8..eb31253201ad 100644
--- a/drivers/usb/core/phy.h
+++ b/drivers/usb/core/phy.h
@@ -1,6 +1,8 @@
 struct usb_phy_roothub;
 
-struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev);
+struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);
+
+int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub);
 int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
 
 int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
-- 
2.16.3

[RFC usb-next v3 1/2] usb: core: split usb_phy_roothub_{init,alloc}

[Martin Blumenstingl]

Before this patch usb_phy_roothub_init served two purposes (from a
caller's point of view - like hcd.c):
- parsing the PHYs and allocating the list entries
- calling phy_init on each list entry

While this worked so far it has one disadvantage: if we need to call
phy_init for each PHY instance then the existing code cannot be re-used.
Solve this by splitting off usb_phy_roothub_alloc which only parses the
PHYs and allocates the list entries.
usb_phy_roothub_init then gets a struct usb_phy_roothub and only calls
phy_init on each PHY instance (along with the corresponding cleanup if
that failed somewhere).

This is a preparation step for adding proper suspend support for some
hardware that requires phy_exit to be called during suspend and phy_init
to be called during resume.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/usb/core/hcd.c | 10 +++++++---
 drivers/usb/core/phy.c | 53 +++++++++++++++++++++++++-------------------------
 drivers/usb/core/phy.h |  4 +++-
 3 files changed, 37 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae6367..15b0418e3b6a 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2758,12 +2758,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	}
 
 	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
-		hcd->phy_roothub = usb_phy_roothub_init(hcd->self.sysdev);
+		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
 		if (IS_ERR(hcd->phy_roothub)) {
 			retval = PTR_ERR(hcd->phy_roothub);
-			goto err_phy_roothub_init;
+			goto err_phy_roothub_alloc;
 		}
 
+		retval = usb_phy_roothub_init(hcd->phy_roothub);
+		if (retval)
+			goto err_phy_roothub_alloc;
+
 		retval = usb_phy_roothub_power_on(hcd->phy_roothub);
 		if (retval)
 			goto err_usb_phy_roothub_power_on;
@@ -2936,7 +2940,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	usb_phy_roothub_power_off(hcd->phy_roothub);
 err_usb_phy_roothub_power_on:
 	usb_phy_roothub_exit(hcd->phy_roothub);
-err_phy_roothub_init:
+err_phy_roothub_alloc:
 	if (hcd->remove_phy && hcd->usb_phy) {
 		usb_phy_shutdown(hcd->usb_phy);
 		usb_put_phy(hcd->usb_phy);
diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
index f19aaa3c899c..44f008cda7a8 100644
--- a/drivers/usb/core/phy.c
+++ b/drivers/usb/core/phy.c
@@ -19,19 +19,6 @@ struct usb_phy_roothub {
 	struct list_head	list;
 };
 
-static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
-{
-	struct usb_phy_roothub *roothub_entry;
-
-	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
-	if (!roothub_entry)
-		return ERR_PTR(-ENOMEM);
-
-	INIT_LIST_HEAD(&roothub_entry->list);
-
-	return roothub_entry;
-}
-
 static int usb_phy_roothub_add_phy(struct device *dev, int index,
 				   struct list_head *list)
 {
@@ -45,9 +32,11 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
 			return PTR_ERR(phy);
 	}
 
-	roothub_entry = usb_phy_roothub_alloc(dev);
-	if (IS_ERR(roothub_entry))
-		return PTR_ERR(roothub_entry);
+	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
+	if (!roothub_entry)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&roothub_entry->list);
 
 	roothub_entry->phy = phy;
 
@@ -56,11 +45,9 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
 	return 0;
 }
 
-struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
+struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
 {
 	struct usb_phy_roothub *phy_roothub;
-	struct usb_phy_roothub *roothub_entry;
-	struct list_head *head;
 	int i, num_phys, err;
 
 	num_phys = of_count_phandle_with_args(dev->of_node, "phys",
@@ -68,16 +55,31 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
 	if (num_phys <= 0)
 		return NULL;
 
-	phy_roothub = usb_phy_roothub_alloc(dev);
-	if (IS_ERR(phy_roothub))
-		return phy_roothub;
+	phy_roothub = devm_kzalloc(dev, sizeof(*phy_roothub), GFP_KERNEL);
+	if (!phy_roothub)
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&phy_roothub->list);
 
 	for (i = 0; i < num_phys; i++) {
 		err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list);
 		if (err)
-			goto err_out;
+			return ERR_PTR(err);
 	}
 
+	return phy_roothub;
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_alloc);
+
+int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub)
+{
+	struct usb_phy_roothub *roothub_entry;
+	struct list_head *head;
+	int err;
+
+	if (!phy_roothub)
+		return 0;
+
 	head = &phy_roothub->list;
 
 	list_for_each_entry(roothub_entry, head, list) {
@@ -86,14 +88,13 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
 			goto err_exit_phys;
 	}
 
-	return phy_roothub;
+	return 0;
 
 err_exit_phys:
 	list_for_each_entry_continue_reverse(roothub_entry, head, list)
 		phy_exit(roothub_entry->phy);
 
-err_out:
-	return ERR_PTR(err);
+	return err;
 }
 EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
 
diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
index 6fde59bfbff8..eb31253201ad 100644
--- a/drivers/usb/core/phy.h
+++ b/drivers/usb/core/phy.h
@@ -1,6 +1,8 @@
 struct usb_phy_roothub;
 
-struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev);
+struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);
+
+int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub);
 int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
 
 int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
-- 
2.16.3

[RFC,usb-next,v3,2/2] usb: core: use phy_exit during suspend if wake up is not supported

[Martin Blumenstingl]

If the USB controller can wake up the system (which is the case for
example with the Mediatek USB3 IP) then we must not call phy_exit during
suspend to ensure that the USB controller doesn't have to re-enumerate
the devices during resume.
However, if the USB controller cannot wake up the system (which is the
case for example on various TI platforms using a dwc3 controller) then
we must call phy_exit during suspend. Otherwise the PHY driver keeps the
clocks enabled, which prevents the system from reaching the lowest power
levels in the suspend state.

Solve this by introducing two new functions in the PHY wrapper which are
dedicated to the suspend and resume handling.
If the controller can wake up the system the new usb_phy_roothub_suspend
function will simply call usb_phy_roothub_power_off. However, if wake up
is not supported by the controller it will also call
usb_phy_roothub_exit.
The also new usb_phy_roothub_resume function takes care of calling
usb_phy_roothub_init (if the controller can't wake up the system) in
addition to usb_phy_roothub_power_on.

Fixes: 07dbff0ddbd86c ("usb: core: add a wrapper for the USB PHYs on the HCD")
Fixes: 178a0bce05cbc1 ("usb: core: hcd: integrate the PHY wrapper into the HCD core")
Reported-by: Roger Quadros <rogerq@ti.com>
Suggested-by: Roger Quadros <rogerq@ti.com>
Suggested-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/usb/core/hcd.c |  8 +++++---
 drivers/usb/core/phy.c | 35 +++++++++++++++++++++++++++++++++++
 drivers/usb/core/phy.h |  5 +++++
 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 15b0418e3b6a..78bae4ecd68b 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2262,7 +2262,8 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
 		hcd->state = HC_STATE_SUSPENDED;
 
 		if (!PMSG_IS_AUTO(msg))
-			usb_phy_roothub_power_off(hcd->phy_roothub);
+			usb_phy_roothub_suspend(hcd->self.sysdev,
+						hcd->phy_roothub);
 
 		/* Did we race with a root-hub wakeup event? */
 		if (rhdev->do_remote_wakeup) {
@@ -2302,7 +2303,8 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 	}
 
 	if (!PMSG_IS_AUTO(msg)) {
-		status = usb_phy_roothub_power_on(hcd->phy_roothub);
+		status = usb_phy_roothub_resume(hcd->self.sysdev,
+						hcd->phy_roothub);
 		if (status)
 			return status;
 	}
@@ -2344,7 +2346,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 		}
 	} else {
 		hcd->state = old_state;
-		usb_phy_roothub_power_off(hcd->phy_roothub);
+		usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
 		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
 				"resume", status);
 		if (status != -ESHUTDOWN)
diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
index 44f008cda7a8..a39d9bb26a4f 100644
--- a/drivers/usb/core/phy.c
+++ b/drivers/usb/core/phy.c
@@ -157,3 +157,38 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
 		phy_power_off(roothub_entry->phy);
 }
 EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
+
+int usb_phy_roothub_suspend(struct device *controller_dev,
+			    struct usb_phy_roothub *phy_roothub)
+{
+	usb_phy_roothub_power_off(phy_roothub);
+
+	/* keep the PHYs initialized so the device can wake up the system */
+	if (device_may_wakeup(controller_dev))
+		return 0;
+
+	return usb_phy_roothub_exit(phy_roothub);
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_suspend);
+
+int usb_phy_roothub_resume(struct device *controller_dev,
+			   struct usb_phy_roothub *phy_roothub)
+{
+	int err;
+
+	/* if the device can't wake up the system _exit was called */
+	if (!device_may_wakeup(controller_dev)) {
+		err = usb_phy_roothub_init(phy_roothub);
+		if (err)
+			return err;
+	}
+
+	err = usb_phy_roothub_power_on(phy_roothub);
+
+	/* undo _init if _power_on failed */
+	if (err && !device_may_wakeup(controller_dev))
+		usb_phy_roothub_exit(phy_roothub);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_resume);
diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
index eb31253201ad..605555901d44 100644
--- a/drivers/usb/core/phy.h
+++ b/drivers/usb/core/phy.h
@@ -7,3 +7,8 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
 
 int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
 void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
+
+int usb_phy_roothub_suspend(struct device *controller_dev,
+			    struct usb_phy_roothub *phy_roothub);
+int usb_phy_roothub_resume(struct device *controller_dev,
+			   struct usb_phy_roothub *phy_roothub);

[RFC usb-next v3 2/2] usb: core: use phy_exit during suspend if wake up is not supported

[Martin Blumenstingl]

If the USB controller can wake up the system (which is the case for
example with the Mediatek USB3 IP) then we must not call phy_exit during
suspend to ensure that the USB controller doesn't have to re-enumerate
the devices during resume.
However, if the USB controller cannot wake up the system (which is the
case for example on various TI platforms using a dwc3 controller) then
we must call phy_exit during suspend. Otherwise the PHY driver keeps the
clocks enabled, which prevents the system from reaching the lowest power
levels in the suspend state.

Solve this by introducing two new functions in the PHY wrapper which are
dedicated to the suspend and resume handling.
If the controller can wake up the system the new usb_phy_roothub_suspend
function will simply call usb_phy_roothub_power_off. However, if wake up
is not supported by the controller it will also call
usb_phy_roothub_exit.
The also new usb_phy_roothub_resume function takes care of calling
usb_phy_roothub_init (if the controller can't wake up the system) in
addition to usb_phy_roothub_power_on.

Fixes: 07dbff0ddbd86c ("usb: core: add a wrapper for the USB PHYs on the HCD")
Fixes: 178a0bce05cbc1 ("usb: core: hcd: integrate the PHY wrapper into the HCD core")
Reported-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
Suggested-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
Suggested-by: Chunfeng Yun <chunfeng.yun-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 drivers/usb/core/hcd.c |  8 +++++---
 drivers/usb/core/phy.c | 35 +++++++++++++++++++++++++++++++++++
 drivers/usb/core/phy.h |  5 +++++
 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 15b0418e3b6a..78bae4ecd68b 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2262,7 +2262,8 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
 		hcd->state = HC_STATE_SUSPENDED;
 
 		if (!PMSG_IS_AUTO(msg))
-			usb_phy_roothub_power_off(hcd->phy_roothub);
+			usb_phy_roothub_suspend(hcd->self.sysdev,
+						hcd->phy_roothub);
 
 		/* Did we race with a root-hub wakeup event? */
 		if (rhdev->do_remote_wakeup) {
@@ -2302,7 +2303,8 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 	}
 
 	if (!PMSG_IS_AUTO(msg)) {
-		status = usb_phy_roothub_power_on(hcd->phy_roothub);
+		status = usb_phy_roothub_resume(hcd->self.sysdev,
+						hcd->phy_roothub);
 		if (status)
 			return status;
 	}
@@ -2344,7 +2346,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 		}
 	} else {
 		hcd->state = old_state;
-		usb_phy_roothub_power_off(hcd->phy_roothub);
+		usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
 		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
 				"resume", status);
 		if (status != -ESHUTDOWN)
diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
index 44f008cda7a8..a39d9bb26a4f 100644
--- a/drivers/usb/core/phy.c
+++ b/drivers/usb/core/phy.c
@@ -157,3 +157,38 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
 		phy_power_off(roothub_entry->phy);
 }
 EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
+
+int usb_phy_roothub_suspend(struct device *controller_dev,
+			    struct usb_phy_roothub *phy_roothub)
+{
+	usb_phy_roothub_power_off(phy_roothub);
+
+	/* keep the PHYs initialized so the device can wake up the system */
+	if (device_may_wakeup(controller_dev))
+		return 0;
+
+	return usb_phy_roothub_exit(phy_roothub);
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_suspend);
+
+int usb_phy_roothub_resume(struct device *controller_dev,
+			   struct usb_phy_roothub *phy_roothub)
+{
+	int err;
+
+	/* if the device can't wake up the system _exit was called */
+	if (!device_may_wakeup(controller_dev)) {
+		err = usb_phy_roothub_init(phy_roothub);
+		if (err)
+			return err;
+	}
+
+	err = usb_phy_roothub_power_on(phy_roothub);
+
+	/* undo _init if _power_on failed */
+	if (err && !device_may_wakeup(controller_dev))
+		usb_phy_roothub_exit(phy_roothub);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_resume);
diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
index eb31253201ad..605555901d44 100644
--- a/drivers/usb/core/phy.h
+++ b/drivers/usb/core/phy.h
@@ -7,3 +7,8 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
 
 int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
 void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
+
+int usb_phy_roothub_suspend(struct device *controller_dev,
+			    struct usb_phy_roothub *phy_roothub);
+int usb_phy_roothub_resume(struct device *controller_dev,
+			   struct usb_phy_roothub *phy_roothub);
-- 
2.16.3

[RFC usb-next v3 2/2] usb: core: use phy_exit during suspend if wake up is not supported

[Martin Blumenstingl]

If the USB controller can wake up the system (which is the case for
example with the Mediatek USB3 IP) then we must not call phy_exit during
suspend to ensure that the USB controller doesn't have to re-enumerate
the devices during resume.
However, if the USB controller cannot wake up the system (which is the
case for example on various TI platforms using a dwc3 controller) then
we must call phy_exit during suspend. Otherwise the PHY driver keeps the
clocks enabled, which prevents the system from reaching the lowest power
levels in the suspend state.

Solve this by introducing two new functions in the PHY wrapper which are
dedicated to the suspend and resume handling.
If the controller can wake up the system the new usb_phy_roothub_suspend
function will simply call usb_phy_roothub_power_off. However, if wake up
is not supported by the controller it will also call
usb_phy_roothub_exit.
The also new usb_phy_roothub_resume function takes care of calling
usb_phy_roothub_init (if the controller can't wake up the system) in
addition to usb_phy_roothub_power_on.

Fixes: 07dbff0ddbd86c ("usb: core: add a wrapper for the USB PHYs on the HCD")
Fixes: 178a0bce05cbc1 ("usb: core: hcd: integrate the PHY wrapper into the HCD core")
Reported-by: Roger Quadros <rogerq@ti.com>
Suggested-by: Roger Quadros <rogerq@ti.com>
Suggested-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/usb/core/hcd.c |  8 +++++---
 drivers/usb/core/phy.c | 35 +++++++++++++++++++++++++++++++++++
 drivers/usb/core/phy.h |  5 +++++
 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 15b0418e3b6a..78bae4ecd68b 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2262,7 +2262,8 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
 		hcd->state = HC_STATE_SUSPENDED;
 
 		if (!PMSG_IS_AUTO(msg))
-			usb_phy_roothub_power_off(hcd->phy_roothub);
+			usb_phy_roothub_suspend(hcd->self.sysdev,
+						hcd->phy_roothub);
 
 		/* Did we race with a root-hub wakeup event? */
 		if (rhdev->do_remote_wakeup) {
@@ -2302,7 +2303,8 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 	}
 
 	if (!PMSG_IS_AUTO(msg)) {
-		status = usb_phy_roothub_power_on(hcd->phy_roothub);
+		status = usb_phy_roothub_resume(hcd->self.sysdev,
+						hcd->phy_roothub);
 		if (status)
 			return status;
 	}
@@ -2344,7 +2346,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 		}
 	} else {
 		hcd->state = old_state;
-		usb_phy_roothub_power_off(hcd->phy_roothub);
+		usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
 		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
 				"resume", status);
 		if (status != -ESHUTDOWN)
diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
index 44f008cda7a8..a39d9bb26a4f 100644
--- a/drivers/usb/core/phy.c
+++ b/drivers/usb/core/phy.c
@@ -157,3 +157,38 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
 		phy_power_off(roothub_entry->phy);
 }
 EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
+
+int usb_phy_roothub_suspend(struct device *controller_dev,
+			    struct usb_phy_roothub *phy_roothub)
+{
+	usb_phy_roothub_power_off(phy_roothub);
+
+	/* keep the PHYs initialized so the device can wake up the system */
+	if (device_may_wakeup(controller_dev))
+		return 0;
+
+	return usb_phy_roothub_exit(phy_roothub);
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_suspend);
+
+int usb_phy_roothub_resume(struct device *controller_dev,
+			   struct usb_phy_roothub *phy_roothub)
+{
+	int err;
+
+	/* if the device can't wake up the system _exit was called */
+	if (!device_may_wakeup(controller_dev)) {
+		err = usb_phy_roothub_init(phy_roothub);
+		if (err)
+			return err;
+	}
+
+	err = usb_phy_roothub_power_on(phy_roothub);
+
+	/* undo _init if _power_on failed */
+	if (err && !device_may_wakeup(controller_dev))
+		usb_phy_roothub_exit(phy_roothub);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_resume);
diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
index eb31253201ad..605555901d44 100644
--- a/drivers/usb/core/phy.h
+++ b/drivers/usb/core/phy.h
@@ -7,3 +7,8 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
 
 int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
 void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
+
+int usb_phy_roothub_suspend(struct device *controller_dev,
+			    struct usb_phy_roothub *phy_roothub);
+int usb_phy_roothub_resume(struct device *controller_dev,
+			   struct usb_phy_roothub *phy_roothub);
-- 
2.16.3

[RFC,usb-next,v3,1/2] usb: core: split usb_phy_roothub_{init,alloc}

[Chunfeng Yun]

hi,
On Mon, 2018-03-26 at 22:38 +0200, Martin Blumenstingl wrote:
> Before this patch usb_phy_roothub_init served two purposes (from a
> caller's point of view - like hcd.c):
> - parsing the PHYs and allocating the list entries
> - calling phy_init on each list entry
> 
> While this worked so far it has one disadvantage: if we need to call
> phy_init for each PHY instance then the existing code cannot be re-used.
> Solve this by splitting off usb_phy_roothub_alloc which only parses the
> PHYs and allocates the list entries.
> usb_phy_roothub_init then gets a struct usb_phy_roothub and only calls
> phy_init on each PHY instance (along with the corresponding cleanup if
> that failed somewhere).
> 
> This is a preparation step for adding proper suspend support for some
> hardware that requires phy_exit to be called during suspend and phy_init
> to be called during resume.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/usb/core/hcd.c | 10 +++++++---
>  drivers/usb/core/phy.c | 53 +++++++++++++++++++++++++-------------------------
>  drivers/usb/core/phy.h |  4 +++-
>  3 files changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 777036ae6367..15b0418e3b6a 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2758,12 +2758,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	}
>  
>  	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
> -		hcd->phy_roothub = usb_phy_roothub_init(hcd->self.sysdev);
> +		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
>  		if (IS_ERR(hcd->phy_roothub)) {
>  			retval = PTR_ERR(hcd->phy_roothub);
> -			goto err_phy_roothub_init;
> +			goto err_phy_roothub_alloc;
>  		}
>  
> +		retval = usb_phy_roothub_init(hcd->phy_roothub);
> +		if (retval)
> +			goto err_phy_roothub_alloc;
> +
>  		retval = usb_phy_roothub_power_on(hcd->phy_roothub);
>  		if (retval)
>  			goto err_usb_phy_roothub_power_on;
> @@ -2936,7 +2940,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	usb_phy_roothub_power_off(hcd->phy_roothub);
>  err_usb_phy_roothub_power_on:
>  	usb_phy_roothub_exit(hcd->phy_roothub);
> -err_phy_roothub_init:
> +err_phy_roothub_alloc:
>  	if (hcd->remove_phy && hcd->usb_phy) {
>  		usb_phy_shutdown(hcd->usb_phy);
>  		usb_put_phy(hcd->usb_phy);
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index f19aaa3c899c..44f008cda7a8 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -19,19 +19,6 @@ struct usb_phy_roothub {
>  	struct list_head	list;
>  };
>  
> -static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
> -{
> -	struct usb_phy_roothub *roothub_entry;
> -
> -	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> -	if (!roothub_entry)
> -		return ERR_PTR(-ENOMEM);
> -
> -	INIT_LIST_HEAD(&roothub_entry->list);
> -
> -	return roothub_entry;
> -}
> -
>  static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  				   struct list_head *list)
>  {
> @@ -45,9 +32,11 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  			return PTR_ERR(phy);
>  	}
>  
> -	roothub_entry = usb_phy_roothub_alloc(dev);
> -	if (IS_ERR(roothub_entry))
> -		return PTR_ERR(roothub_entry);
> +	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> +	if (!roothub_entry)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&roothub_entry->list);
>  
>  	roothub_entry->phy = phy;
>  
> @@ -56,11 +45,9 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  	return 0;
>  }
>  
> -struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
>  {
>  	struct usb_phy_roothub *phy_roothub;
> -	struct usb_phy_roothub *roothub_entry;
> -	struct list_head *head;
>  	int i, num_phys, err;
>  
>  	num_phys = of_count_phandle_with_args(dev->of_node, "phys",
> @@ -68,16 +55,31 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>  	if (num_phys <= 0)
>  		return NULL;
>  
> -	phy_roothub = usb_phy_roothub_alloc(dev);
> -	if (IS_ERR(phy_roothub))
> -		return phy_roothub;
> +	phy_roothub = devm_kzalloc(dev, sizeof(*phy_roothub), GFP_KERNEL);
> +	if (!phy_roothub)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&phy_roothub->list);
>  
>  	for (i = 0; i < num_phys; i++) {
>  		err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list);
>  		if (err)
> -			goto err_out;
> +			return ERR_PTR(err);
>  	}
>  
> +	return phy_roothub;
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_alloc);
> +
> +int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub)
> +{
> +	struct usb_phy_roothub *roothub_entry;
> +	struct list_head *head;
> +	int err;
> +
> +	if (!phy_roothub)
> +		return 0;
> +
>  	head = &phy_roothub->list;
>  
>  	list_for_each_entry(roothub_entry, head, list) {
> @@ -86,14 +88,13 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>  			goto err_exit_phys;
>  	}
>  
> -	return phy_roothub;
> +	return 0;
>  
>  err_exit_phys:
>  	list_for_each_entry_continue_reverse(roothub_entry, head, list)
>  		phy_exit(roothub_entry->phy);
>  
> -err_out:
> -	return ERR_PTR(err);
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
>  
> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> index 6fde59bfbff8..eb31253201ad 100644
> --- a/drivers/usb/core/phy.h
> +++ b/drivers/usb/core/phy.h
> @@ -1,6 +1,8 @@
>  struct usb_phy_roothub;
>  
> -struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev);
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);
> +
> +int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub);
>  int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
>  
>  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);

Tested-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
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

Re: [RFC usb-next v3 1/2] usb: core: split usb_phy_roothub_{init,alloc}

[Chunfeng Yun]

hi,
On Mon, 2018-03-26 at 22:38 +0200, Martin Blumenstingl wrote:
> Before this patch usb_phy_roothub_init served two purposes (from a
> caller's point of view - like hcd.c):
> - parsing the PHYs and allocating the list entries
> - calling phy_init on each list entry
> 
> While this worked so far it has one disadvantage: if we need to call
> phy_init for each PHY instance then the existing code cannot be re-used.
> Solve this by splitting off usb_phy_roothub_alloc which only parses the
> PHYs and allocates the list entries.
> usb_phy_roothub_init then gets a struct usb_phy_roothub and only calls
> phy_init on each PHY instance (along with the corresponding cleanup if
> that failed somewhere).
> 
> This is a preparation step for adding proper suspend support for some
> hardware that requires phy_exit to be called during suspend and phy_init
> to be called during resume.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> ---
>  drivers/usb/core/hcd.c | 10 +++++++---
>  drivers/usb/core/phy.c | 53 +++++++++++++++++++++++++-------------------------
>  drivers/usb/core/phy.h |  4 +++-
>  3 files changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 777036ae6367..15b0418e3b6a 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2758,12 +2758,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	}
>  
>  	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
> -		hcd->phy_roothub = usb_phy_roothub_init(hcd->self.sysdev);
> +		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
>  		if (IS_ERR(hcd->phy_roothub)) {
>  			retval = PTR_ERR(hcd->phy_roothub);
> -			goto err_phy_roothub_init;
> +			goto err_phy_roothub_alloc;
>  		}
>  
> +		retval = usb_phy_roothub_init(hcd->phy_roothub);
> +		if (retval)
> +			goto err_phy_roothub_alloc;
> +
>  		retval = usb_phy_roothub_power_on(hcd->phy_roothub);
>  		if (retval)
>  			goto err_usb_phy_roothub_power_on;
> @@ -2936,7 +2940,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	usb_phy_roothub_power_off(hcd->phy_roothub);
>  err_usb_phy_roothub_power_on:
>  	usb_phy_roothub_exit(hcd->phy_roothub);
> -err_phy_roothub_init:
> +err_phy_roothub_alloc:
>  	if (hcd->remove_phy && hcd->usb_phy) {
>  		usb_phy_shutdown(hcd->usb_phy);
>  		usb_put_phy(hcd->usb_phy);
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index f19aaa3c899c..44f008cda7a8 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -19,19 +19,6 @@ struct usb_phy_roothub {
>  	struct list_head	list;
>  };
>  
> -static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
> -{
> -	struct usb_phy_roothub *roothub_entry;
> -
> -	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> -	if (!roothub_entry)
> -		return ERR_PTR(-ENOMEM);
> -
> -	INIT_LIST_HEAD(&roothub_entry->list);
> -
> -	return roothub_entry;
> -}
> -
>  static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  				   struct list_head *list)
>  {
> @@ -45,9 +32,11 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  			return PTR_ERR(phy);
>  	}
>  
> -	roothub_entry = usb_phy_roothub_alloc(dev);
> -	if (IS_ERR(roothub_entry))
> -		return PTR_ERR(roothub_entry);
> +	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> +	if (!roothub_entry)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&roothub_entry->list);
>  
>  	roothub_entry->phy = phy;
>  
> @@ -56,11 +45,9 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  	return 0;
>  }
>  
> -struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
>  {
>  	struct usb_phy_roothub *phy_roothub;
> -	struct usb_phy_roothub *roothub_entry;
> -	struct list_head *head;
>  	int i, num_phys, err;
>  
>  	num_phys = of_count_phandle_with_args(dev->of_node, "phys",
> @@ -68,16 +55,31 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>  	if (num_phys <= 0)
>  		return NULL;
>  
> -	phy_roothub = usb_phy_roothub_alloc(dev);
> -	if (IS_ERR(phy_roothub))
> -		return phy_roothub;
> +	phy_roothub = devm_kzalloc(dev, sizeof(*phy_roothub), GFP_KERNEL);
> +	if (!phy_roothub)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&phy_roothub->list);
>  
>  	for (i = 0; i < num_phys; i++) {
>  		err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list);
>  		if (err)
> -			goto err_out;
> +			return ERR_PTR(err);
>  	}
>  
> +	return phy_roothub;
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_alloc);
> +
> +int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub)
> +{
> +	struct usb_phy_roothub *roothub_entry;
> +	struct list_head *head;
> +	int err;
> +
> +	if (!phy_roothub)
> +		return 0;
> +
>  	head = &phy_roothub->list;
>  
>  	list_for_each_entry(roothub_entry, head, list) {
> @@ -86,14 +88,13 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>  			goto err_exit_phys;
>  	}
>  
> -	return phy_roothub;
> +	return 0;
>  
>  err_exit_phys:
>  	list_for_each_entry_continue_reverse(roothub_entry, head, list)
>  		phy_exit(roothub_entry->phy);
>  
> -err_out:
> -	return ERR_PTR(err);
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
>  
> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> index 6fde59bfbff8..eb31253201ad 100644
> --- a/drivers/usb/core/phy.h
> +++ b/drivers/usb/core/phy.h
> @@ -1,6 +1,8 @@
>  struct usb_phy_roothub;
>  
> -struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev);
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);
> +
> +int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub);
>  int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
>  
>  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);

Tested-by: Chunfeng Yun <chunfeng.yun-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

[RFC usb-next v3 1/2] usb: core: split usb_phy_roothub_{init,alloc}

[Chunfeng Yun]

hi,
On Mon, 2018-03-26 at 22:38 +0200, Martin Blumenstingl wrote:
> Before this patch usb_phy_roothub_init served two purposes (from a
> caller's point of view - like hcd.c):
> - parsing the PHYs and allocating the list entries
> - calling phy_init on each list entry
> 
> While this worked so far it has one disadvantage: if we need to call
> phy_init for each PHY instance then the existing code cannot be re-used.
> Solve this by splitting off usb_phy_roothub_alloc which only parses the
> PHYs and allocates the list entries.
> usb_phy_roothub_init then gets a struct usb_phy_roothub and only calls
> phy_init on each PHY instance (along with the corresponding cleanup if
> that failed somewhere).
> 
> This is a preparation step for adding proper suspend support for some
> hardware that requires phy_exit to be called during suspend and phy_init
> to be called during resume.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/usb/core/hcd.c | 10 +++++++---
>  drivers/usb/core/phy.c | 53 +++++++++++++++++++++++++-------------------------
>  drivers/usb/core/phy.h |  4 +++-
>  3 files changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 777036ae6367..15b0418e3b6a 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2758,12 +2758,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	}
>  
>  	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
> -		hcd->phy_roothub = usb_phy_roothub_init(hcd->self.sysdev);
> +		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
>  		if (IS_ERR(hcd->phy_roothub)) {
>  			retval = PTR_ERR(hcd->phy_roothub);
> -			goto err_phy_roothub_init;
> +			goto err_phy_roothub_alloc;
>  		}
>  
> +		retval = usb_phy_roothub_init(hcd->phy_roothub);
> +		if (retval)
> +			goto err_phy_roothub_alloc;
> +
>  		retval = usb_phy_roothub_power_on(hcd->phy_roothub);
>  		if (retval)
>  			goto err_usb_phy_roothub_power_on;
> @@ -2936,7 +2940,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	usb_phy_roothub_power_off(hcd->phy_roothub);
>  err_usb_phy_roothub_power_on:
>  	usb_phy_roothub_exit(hcd->phy_roothub);
> -err_phy_roothub_init:
> +err_phy_roothub_alloc:
>  	if (hcd->remove_phy && hcd->usb_phy) {
>  		usb_phy_shutdown(hcd->usb_phy);
>  		usb_put_phy(hcd->usb_phy);
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index f19aaa3c899c..44f008cda7a8 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -19,19 +19,6 @@ struct usb_phy_roothub {
>  	struct list_head	list;
>  };
>  
> -static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
> -{
> -	struct usb_phy_roothub *roothub_entry;
> -
> -	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> -	if (!roothub_entry)
> -		return ERR_PTR(-ENOMEM);
> -
> -	INIT_LIST_HEAD(&roothub_entry->list);
> -
> -	return roothub_entry;
> -}
> -
>  static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  				   struct list_head *list)
>  {
> @@ -45,9 +32,11 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  			return PTR_ERR(phy);
>  	}
>  
> -	roothub_entry = usb_phy_roothub_alloc(dev);
> -	if (IS_ERR(roothub_entry))
> -		return PTR_ERR(roothub_entry);
> +	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> +	if (!roothub_entry)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&roothub_entry->list);
>  
>  	roothub_entry->phy = phy;
>  
> @@ -56,11 +45,9 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  	return 0;
>  }
>  
> -struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
>  {
>  	struct usb_phy_roothub *phy_roothub;
> -	struct usb_phy_roothub *roothub_entry;
> -	struct list_head *head;
>  	int i, num_phys, err;
>  
>  	num_phys = of_count_phandle_with_args(dev->of_node, "phys",
> @@ -68,16 +55,31 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>  	if (num_phys <= 0)
>  		return NULL;
>  
> -	phy_roothub = usb_phy_roothub_alloc(dev);
> -	if (IS_ERR(phy_roothub))
> -		return phy_roothub;
> +	phy_roothub = devm_kzalloc(dev, sizeof(*phy_roothub), GFP_KERNEL);
> +	if (!phy_roothub)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&phy_roothub->list);
>  
>  	for (i = 0; i < num_phys; i++) {
>  		err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list);
>  		if (err)
> -			goto err_out;
> +			return ERR_PTR(err);
>  	}
>  
> +	return phy_roothub;
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_alloc);
> +
> +int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub)
> +{
> +	struct usb_phy_roothub *roothub_entry;
> +	struct list_head *head;
> +	int err;
> +
> +	if (!phy_roothub)
> +		return 0;
> +
>  	head = &phy_roothub->list;
>  
>  	list_for_each_entry(roothub_entry, head, list) {
> @@ -86,14 +88,13 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>  			goto err_exit_phys;
>  	}
>  
> -	return phy_roothub;
> +	return 0;
>  
>  err_exit_phys:
>  	list_for_each_entry_continue_reverse(roothub_entry, head, list)
>  		phy_exit(roothub_entry->phy);
>  
> -err_out:
> -	return ERR_PTR(err);
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
>  
> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> index 6fde59bfbff8..eb31253201ad 100644
> --- a/drivers/usb/core/phy.h
> +++ b/drivers/usb/core/phy.h
> @@ -1,6 +1,8 @@
>  struct usb_phy_roothub;
>  
> -struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev);
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);
> +
> +int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub);
>  int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
>  
>  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);

Tested-by: Chunfeng Yun <chunfeng.yun@mediatek.com>

[RFC,usb-next,v3,2/2] usb: core: use phy_exit during suspend if wake up is not supported

[Chunfeng Yun]

hi,
On Mon, 2018-03-26 at 22:38 +0200, Martin Blumenstingl wrote:
> If the USB controller can wake up the system (which is the case for
> example with the Mediatek USB3 IP) then we must not call phy_exit during
> suspend to ensure that the USB controller doesn't have to re-enumerate
> the devices during resume.
> However, if the USB controller cannot wake up the system (which is the
> case for example on various TI platforms using a dwc3 controller) then
> we must call phy_exit during suspend. Otherwise the PHY driver keeps the
> clocks enabled, which prevents the system from reaching the lowest power
> levels in the suspend state.
> 
> Solve this by introducing two new functions in the PHY wrapper which are
> dedicated to the suspend and resume handling.
> If the controller can wake up the system the new usb_phy_roothub_suspend
> function will simply call usb_phy_roothub_power_off. However, if wake up
> is not supported by the controller it will also call
> usb_phy_roothub_exit.
> The also new usb_phy_roothub_resume function takes care of calling
> usb_phy_roothub_init (if the controller can't wake up the system) in
> addition to usb_phy_roothub_power_on.
> 
> Fixes: 07dbff0ddbd86c ("usb: core: add a wrapper for the USB PHYs on the HCD")
> Fixes: 178a0bce05cbc1 ("usb: core: hcd: integrate the PHY wrapper into the HCD core")
> Reported-by: Roger Quadros <rogerq@ti.com>
> Suggested-by: Roger Quadros <rogerq@ti.com>
> Suggested-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/usb/core/hcd.c |  8 +++++---
>  drivers/usb/core/phy.c | 35 +++++++++++++++++++++++++++++++++++
>  drivers/usb/core/phy.h |  5 +++++
>  3 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 15b0418e3b6a..78bae4ecd68b 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2262,7 +2262,8 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
>  		hcd->state = HC_STATE_SUSPENDED;
>  
>  		if (!PMSG_IS_AUTO(msg))
> -			usb_phy_roothub_power_off(hcd->phy_roothub);
> +			usb_phy_roothub_suspend(hcd->self.sysdev,
> +						hcd->phy_roothub);
>  
>  		/* Did we race with a root-hub wakeup event? */
>  		if (rhdev->do_remote_wakeup) {
> @@ -2302,7 +2303,8 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  	}
>  
>  	if (!PMSG_IS_AUTO(msg)) {
> -		status = usb_phy_roothub_power_on(hcd->phy_roothub);
> +		status = usb_phy_roothub_resume(hcd->self.sysdev,
> +						hcd->phy_roothub);
>  		if (status)
>  			return status;
>  	}
> @@ -2344,7 +2346,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  		}
>  	} else {
>  		hcd->state = old_state;
> -		usb_phy_roothub_power_off(hcd->phy_roothub);
> +		usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
>  		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
>  				"resume", status);
>  		if (status != -ESHUTDOWN)
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index 44f008cda7a8..a39d9bb26a4f 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -157,3 +157,38 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
>  		phy_power_off(roothub_entry->phy);
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
> +
> +int usb_phy_roothub_suspend(struct device *controller_dev,
> +			    struct usb_phy_roothub *phy_roothub)
> +{
> +	usb_phy_roothub_power_off(phy_roothub);
> +
> +	/* keep the PHYs initialized so the device can wake up the system */
> +	if (device_may_wakeup(controller_dev))
> +		return 0;
> +
> +	return usb_phy_roothub_exit(phy_roothub);
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_suspend);
> +
> +int usb_phy_roothub_resume(struct device *controller_dev,
> +			   struct usb_phy_roothub *phy_roothub)
> +{
> +	int err;
> +
> +	/* if the device can't wake up the system _exit was called */
> +	if (!device_may_wakeup(controller_dev)) {
> +		err = usb_phy_roothub_init(phy_roothub);
> +		if (err)
> +			return err;
> +	}
> +
> +	err = usb_phy_roothub_power_on(phy_roothub);
> +
> +	/* undo _init if _power_on failed */
> +	if (err && !device_may_wakeup(controller_dev))
> +		usb_phy_roothub_exit(phy_roothub);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_resume);
> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> index eb31253201ad..605555901d44 100644
> --- a/drivers/usb/core/phy.h
> +++ b/drivers/usb/core/phy.h
> @@ -7,3 +7,8 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
>  
>  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
>  void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
> +
> +int usb_phy_roothub_suspend(struct device *controller_dev,
> +			    struct usb_phy_roothub *phy_roothub);
> +int usb_phy_roothub_resume(struct device *controller_dev,
> +			   struct usb_phy_roothub *phy_roothub);

Tested-by: Chunfeng Yun <chunfeng.yun@mediatek.com>

Thanks a lot
---
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

Re: [RFC usb-next v3 2/2] usb: core: use phy_exit during suspend if wake up is not supported

[Chunfeng Yun]

hi,
On Mon, 2018-03-26 at 22:38 +0200, Martin Blumenstingl wrote:
> If the USB controller can wake up the system (which is the case for
> example with the Mediatek USB3 IP) then we must not call phy_exit during
> suspend to ensure that the USB controller doesn't have to re-enumerate
> the devices during resume.
> However, if the USB controller cannot wake up the system (which is the
> case for example on various TI platforms using a dwc3 controller) then
> we must call phy_exit during suspend. Otherwise the PHY driver keeps the
> clocks enabled, which prevents the system from reaching the lowest power
> levels in the suspend state.
> 
> Solve this by introducing two new functions in the PHY wrapper which are
> dedicated to the suspend and resume handling.
> If the controller can wake up the system the new usb_phy_roothub_suspend
> function will simply call usb_phy_roothub_power_off. However, if wake up
> is not supported by the controller it will also call
> usb_phy_roothub_exit.
> The also new usb_phy_roothub_resume function takes care of calling
> usb_phy_roothub_init (if the controller can't wake up the system) in
> addition to usb_phy_roothub_power_on.
> 
> Fixes: 07dbff0ddbd86c ("usb: core: add a wrapper for the USB PHYs on the HCD")
> Fixes: 178a0bce05cbc1 ("usb: core: hcd: integrate the PHY wrapper into the HCD core")
> Reported-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
> Suggested-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
> Suggested-by: Chunfeng Yun <chunfeng.yun-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> ---
>  drivers/usb/core/hcd.c |  8 +++++---
>  drivers/usb/core/phy.c | 35 +++++++++++++++++++++++++++++++++++
>  drivers/usb/core/phy.h |  5 +++++
>  3 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 15b0418e3b6a..78bae4ecd68b 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2262,7 +2262,8 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
>  		hcd->state = HC_STATE_SUSPENDED;
>  
>  		if (!PMSG_IS_AUTO(msg))
> -			usb_phy_roothub_power_off(hcd->phy_roothub);
> +			usb_phy_roothub_suspend(hcd->self.sysdev,
> +						hcd->phy_roothub);
>  
>  		/* Did we race with a root-hub wakeup event? */
>  		if (rhdev->do_remote_wakeup) {
> @@ -2302,7 +2303,8 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  	}
>  
>  	if (!PMSG_IS_AUTO(msg)) {
> -		status = usb_phy_roothub_power_on(hcd->phy_roothub);
> +		status = usb_phy_roothub_resume(hcd->self.sysdev,
> +						hcd->phy_roothub);
>  		if (status)
>  			return status;
>  	}
> @@ -2344,7 +2346,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  		}
>  	} else {
>  		hcd->state = old_state;
> -		usb_phy_roothub_power_off(hcd->phy_roothub);
> +		usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
>  		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
>  				"resume", status);
>  		if (status != -ESHUTDOWN)
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index 44f008cda7a8..a39d9bb26a4f 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -157,3 +157,38 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
>  		phy_power_off(roothub_entry->phy);
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
> +
> +int usb_phy_roothub_suspend(struct device *controller_dev,
> +			    struct usb_phy_roothub *phy_roothub)
> +{
> +	usb_phy_roothub_power_off(phy_roothub);
> +
> +	/* keep the PHYs initialized so the device can wake up the system */
> +	if (device_may_wakeup(controller_dev))
> +		return 0;
> +
> +	return usb_phy_roothub_exit(phy_roothub);
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_suspend);
> +
> +int usb_phy_roothub_resume(struct device *controller_dev,
> +			   struct usb_phy_roothub *phy_roothub)
> +{
> +	int err;
> +
> +	/* if the device can't wake up the system _exit was called */
> +	if (!device_may_wakeup(controller_dev)) {
> +		err = usb_phy_roothub_init(phy_roothub);
> +		if (err)
> +			return err;
> +	}
> +
> +	err = usb_phy_roothub_power_on(phy_roothub);
> +
> +	/* undo _init if _power_on failed */
> +	if (err && !device_may_wakeup(controller_dev))
> +		usb_phy_roothub_exit(phy_roothub);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_resume);
> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> index eb31253201ad..605555901d44 100644
> --- a/drivers/usb/core/phy.h
> +++ b/drivers/usb/core/phy.h
> @@ -7,3 +7,8 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
>  
>  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
>  void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
> +
> +int usb_phy_roothub_suspend(struct device *controller_dev,
> +			    struct usb_phy_roothub *phy_roothub);
> +int usb_phy_roothub_resume(struct device *controller_dev,
> +			   struct usb_phy_roothub *phy_roothub);

Tested-by: Chunfeng Yun <chunfeng.yun-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

Thanks a lot

[RFC usb-next v3 2/2] usb: core: use phy_exit during suspend if wake up is not supported

[Chunfeng Yun]

hi,
On Mon, 2018-03-26 at 22:38 +0200, Martin Blumenstingl wrote:
> If the USB controller can wake up the system (which is the case for
> example with the Mediatek USB3 IP) then we must not call phy_exit during
> suspend to ensure that the USB controller doesn't have to re-enumerate
> the devices during resume.
> However, if the USB controller cannot wake up the system (which is the
> case for example on various TI platforms using a dwc3 controller) then
> we must call phy_exit during suspend. Otherwise the PHY driver keeps the
> clocks enabled, which prevents the system from reaching the lowest power
> levels in the suspend state.
> 
> Solve this by introducing two new functions in the PHY wrapper which are
> dedicated to the suspend and resume handling.
> If the controller can wake up the system the new usb_phy_roothub_suspend
> function will simply call usb_phy_roothub_power_off. However, if wake up
> is not supported by the controller it will also call
> usb_phy_roothub_exit.
> The also new usb_phy_roothub_resume function takes care of calling
> usb_phy_roothub_init (if the controller can't wake up the system) in
> addition to usb_phy_roothub_power_on.
> 
> Fixes: 07dbff0ddbd86c ("usb: core: add a wrapper for the USB PHYs on the HCD")
> Fixes: 178a0bce05cbc1 ("usb: core: hcd: integrate the PHY wrapper into the HCD core")
> Reported-by: Roger Quadros <rogerq@ti.com>
> Suggested-by: Roger Quadros <rogerq@ti.com>
> Suggested-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/usb/core/hcd.c |  8 +++++---
>  drivers/usb/core/phy.c | 35 +++++++++++++++++++++++++++++++++++
>  drivers/usb/core/phy.h |  5 +++++
>  3 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 15b0418e3b6a..78bae4ecd68b 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2262,7 +2262,8 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
>  		hcd->state = HC_STATE_SUSPENDED;
>  
>  		if (!PMSG_IS_AUTO(msg))
> -			usb_phy_roothub_power_off(hcd->phy_roothub);
> +			usb_phy_roothub_suspend(hcd->self.sysdev,
> +						hcd->phy_roothub);
>  
>  		/* Did we race with a root-hub wakeup event? */
>  		if (rhdev->do_remote_wakeup) {
> @@ -2302,7 +2303,8 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  	}
>  
>  	if (!PMSG_IS_AUTO(msg)) {
> -		status = usb_phy_roothub_power_on(hcd->phy_roothub);
> +		status = usb_phy_roothub_resume(hcd->self.sysdev,
> +						hcd->phy_roothub);
>  		if (status)
>  			return status;
>  	}
> @@ -2344,7 +2346,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  		}
>  	} else {
>  		hcd->state = old_state;
> -		usb_phy_roothub_power_off(hcd->phy_roothub);
> +		usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
>  		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
>  				"resume", status);
>  		if (status != -ESHUTDOWN)
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index 44f008cda7a8..a39d9bb26a4f 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -157,3 +157,38 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
>  		phy_power_off(roothub_entry->phy);
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
> +
> +int usb_phy_roothub_suspend(struct device *controller_dev,
> +			    struct usb_phy_roothub *phy_roothub)
> +{
> +	usb_phy_roothub_power_off(phy_roothub);
> +
> +	/* keep the PHYs initialized so the device can wake up the system */
> +	if (device_may_wakeup(controller_dev))
> +		return 0;
> +
> +	return usb_phy_roothub_exit(phy_roothub);
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_suspend);
> +
> +int usb_phy_roothub_resume(struct device *controller_dev,
> +			   struct usb_phy_roothub *phy_roothub)
> +{
> +	int err;
> +
> +	/* if the device can't wake up the system _exit was called */
> +	if (!device_may_wakeup(controller_dev)) {
> +		err = usb_phy_roothub_init(phy_roothub);
> +		if (err)
> +			return err;
> +	}
> +
> +	err = usb_phy_roothub_power_on(phy_roothub);
> +
> +	/* undo _init if _power_on failed */
> +	if (err && !device_may_wakeup(controller_dev))
> +		usb_phy_roothub_exit(phy_roothub);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_resume);
> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> index eb31253201ad..605555901d44 100644
> --- a/drivers/usb/core/phy.h
> +++ b/drivers/usb/core/phy.h
> @@ -7,3 +7,8 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
>  
>  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
>  void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
> +
> +int usb_phy_roothub_suspend(struct device *controller_dev,
> +			    struct usb_phy_roothub *phy_roothub);
> +int usb_phy_roothub_resume(struct device *controller_dev,
> +			   struct usb_phy_roothub *phy_roothub);

Tested-by: Chunfeng Yun <chunfeng.yun@mediatek.com>

Thanks a lot

[RFC,usb-next,v3,1/2] usb: core: split usb_phy_roothub_{init,alloc}

[Roger Quadros]

On 26/03/18 23:38, Martin Blumenstingl wrote:
> Before this patch usb_phy_roothub_init served two purposes (from a
> caller's point of view - like hcd.c):
> - parsing the PHYs and allocating the list entries
> - calling phy_init on each list entry
> 
> While this worked so far it has one disadvantage: if we need to call
> phy_init for each PHY instance then the existing code cannot be re-used.
> Solve this by splitting off usb_phy_roothub_alloc which only parses the
> PHYs and allocates the list entries.
> usb_phy_roothub_init then gets a struct usb_phy_roothub and only calls
> phy_init on each PHY instance (along with the corresponding cleanup if
> that failed somewhere).
> 
> This is a preparation step for adding proper suspend support for some
> hardware that requires phy_exit to be called during suspend and phy_init
> to be called during resume.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

I don't think we need RFC in subject.

Reviewed-by: Roger Quadros <rogerq@ti.com>

> ---
>  drivers/usb/core/hcd.c | 10 +++++++---
>  drivers/usb/core/phy.c | 53 +++++++++++++++++++++++++-------------------------
>  drivers/usb/core/phy.h |  4 +++-
>  3 files changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 777036ae6367..15b0418e3b6a 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2758,12 +2758,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	}
>  
>  	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
> -		hcd->phy_roothub = usb_phy_roothub_init(hcd->self.sysdev);
> +		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
>  		if (IS_ERR(hcd->phy_roothub)) {
>  			retval = PTR_ERR(hcd->phy_roothub);
> -			goto err_phy_roothub_init;
> +			goto err_phy_roothub_alloc;
>  		}
>  
> +		retval = usb_phy_roothub_init(hcd->phy_roothub);
> +		if (retval)
> +			goto err_phy_roothub_alloc;
> +
>  		retval = usb_phy_roothub_power_on(hcd->phy_roothub);
>  		if (retval)
>  			goto err_usb_phy_roothub_power_on;
> @@ -2936,7 +2940,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	usb_phy_roothub_power_off(hcd->phy_roothub);
>  err_usb_phy_roothub_power_on:
>  	usb_phy_roothub_exit(hcd->phy_roothub);
> -err_phy_roothub_init:
> +err_phy_roothub_alloc:
>  	if (hcd->remove_phy && hcd->usb_phy) {
>  		usb_phy_shutdown(hcd->usb_phy);
>  		usb_put_phy(hcd->usb_phy);
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index f19aaa3c899c..44f008cda7a8 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -19,19 +19,6 @@ struct usb_phy_roothub {
>  	struct list_head	list;
>  };
>  
> -static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
> -{
> -	struct usb_phy_roothub *roothub_entry;
> -
> -	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> -	if (!roothub_entry)
> -		return ERR_PTR(-ENOMEM);
> -
> -	INIT_LIST_HEAD(&roothub_entry->list);
> -
> -	return roothub_entry;
> -}
> -
>  static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  				   struct list_head *list)
>  {
> @@ -45,9 +32,11 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  			return PTR_ERR(phy);
>  	}
>  
> -	roothub_entry = usb_phy_roothub_alloc(dev);
> -	if (IS_ERR(roothub_entry))
> -		return PTR_ERR(roothub_entry);
> +	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> +	if (!roothub_entry)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&roothub_entry->list);
>  
>  	roothub_entry->phy = phy;
>  
> @@ -56,11 +45,9 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  	return 0;
>  }
>  
> -struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
>  {
>  	struct usb_phy_roothub *phy_roothub;
> -	struct usb_phy_roothub *roothub_entry;
> -	struct list_head *head;
>  	int i, num_phys, err;
>  
>  	num_phys = of_count_phandle_with_args(dev->of_node, "phys",
> @@ -68,16 +55,31 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>  	if (num_phys <= 0)
>  		return NULL;
>  
> -	phy_roothub = usb_phy_roothub_alloc(dev);
> -	if (IS_ERR(phy_roothub))
> -		return phy_roothub;
> +	phy_roothub = devm_kzalloc(dev, sizeof(*phy_roothub), GFP_KERNEL);
> +	if (!phy_roothub)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&phy_roothub->list);
>  
>  	for (i = 0; i < num_phys; i++) {
>  		err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list);
>  		if (err)
> -			goto err_out;
> +			return ERR_PTR(err);
>  	}
>  
> +	return phy_roothub;
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_alloc);
> +
> +int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub)
> +{
> +	struct usb_phy_roothub *roothub_entry;
> +	struct list_head *head;
> +	int err;
> +
> +	if (!phy_roothub)
> +		return 0;
> +
>  	head = &phy_roothub->list;
>  
>  	list_for_each_entry(roothub_entry, head, list) {
> @@ -86,14 +88,13 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>  			goto err_exit_phys;
>  	}
>  
> -	return phy_roothub;
> +	return 0;
>  
>  err_exit_phys:
>  	list_for_each_entry_continue_reverse(roothub_entry, head, list)
>  		phy_exit(roothub_entry->phy);
>  
> -err_out:
> -	return ERR_PTR(err);
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
>  
> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> index 6fde59bfbff8..eb31253201ad 100644
> --- a/drivers/usb/core/phy.h
> +++ b/drivers/usb/core/phy.h
> @@ -1,6 +1,8 @@
>  struct usb_phy_roothub;
>  
> -struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev);
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);
> +
> +int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub);
>  int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
>  
>  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
>

Re: [RFC usb-next v3 1/2] usb: core: split usb_phy_roothub_{init,alloc}

[Roger Quadros]

On 26/03/18 23:38, Martin Blumenstingl wrote:
> Before this patch usb_phy_roothub_init served two purposes (from a
> caller's point of view - like hcd.c):
> - parsing the PHYs and allocating the list entries
> - calling phy_init on each list entry
> 
> While this worked so far it has one disadvantage: if we need to call
> phy_init for each PHY instance then the existing code cannot be re-used.
> Solve this by splitting off usb_phy_roothub_alloc which only parses the
> PHYs and allocates the list entries.
> usb_phy_roothub_init then gets a struct usb_phy_roothub and only calls
> phy_init on each PHY instance (along with the corresponding cleanup if
> that failed somewhere).
> 
> This is a preparation step for adding proper suspend support for some
> hardware that requires phy_exit to be called during suspend and phy_init
> to be called during resume.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>

I don't think we need RFC in subject.

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

> ---
>  drivers/usb/core/hcd.c | 10 +++++++---
>  drivers/usb/core/phy.c | 53 +++++++++++++++++++++++++-------------------------
>  drivers/usb/core/phy.h |  4 +++-
>  3 files changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 777036ae6367..15b0418e3b6a 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2758,12 +2758,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	}
>  
>  	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
> -		hcd->phy_roothub = usb_phy_roothub_init(hcd->self.sysdev);
> +		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
>  		if (IS_ERR(hcd->phy_roothub)) {
>  			retval = PTR_ERR(hcd->phy_roothub);
> -			goto err_phy_roothub_init;
> +			goto err_phy_roothub_alloc;
>  		}
>  
> +		retval = usb_phy_roothub_init(hcd->phy_roothub);
> +		if (retval)
> +			goto err_phy_roothub_alloc;
> +
>  		retval = usb_phy_roothub_power_on(hcd->phy_roothub);
>  		if (retval)
>  			goto err_usb_phy_roothub_power_on;
> @@ -2936,7 +2940,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	usb_phy_roothub_power_off(hcd->phy_roothub);
>  err_usb_phy_roothub_power_on:
>  	usb_phy_roothub_exit(hcd->phy_roothub);
> -err_phy_roothub_init:
> +err_phy_roothub_alloc:
>  	if (hcd->remove_phy && hcd->usb_phy) {
>  		usb_phy_shutdown(hcd->usb_phy);
>  		usb_put_phy(hcd->usb_phy);
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index f19aaa3c899c..44f008cda7a8 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -19,19 +19,6 @@ struct usb_phy_roothub {
>  	struct list_head	list;
>  };
>  
> -static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
> -{
> -	struct usb_phy_roothub *roothub_entry;
> -
> -	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> -	if (!roothub_entry)
> -		return ERR_PTR(-ENOMEM);
> -
> -	INIT_LIST_HEAD(&roothub_entry->list);
> -
> -	return roothub_entry;
> -}
> -
>  static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  				   struct list_head *list)
>  {
> @@ -45,9 +32,11 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  			return PTR_ERR(phy);
>  	}
>  
> -	roothub_entry = usb_phy_roothub_alloc(dev);
> -	if (IS_ERR(roothub_entry))
> -		return PTR_ERR(roothub_entry);
> +	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> +	if (!roothub_entry)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&roothub_entry->list);
>  
>  	roothub_entry->phy = phy;
>  
> @@ -56,11 +45,9 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  	return 0;
>  }
>  
> -struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
>  {
>  	struct usb_phy_roothub *phy_roothub;
> -	struct usb_phy_roothub *roothub_entry;
> -	struct list_head *head;
>  	int i, num_phys, err;
>  
>  	num_phys = of_count_phandle_with_args(dev->of_node, "phys",
> @@ -68,16 +55,31 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>  	if (num_phys <= 0)
>  		return NULL;
>  
> -	phy_roothub = usb_phy_roothub_alloc(dev);
> -	if (IS_ERR(phy_roothub))
> -		return phy_roothub;
> +	phy_roothub = devm_kzalloc(dev, sizeof(*phy_roothub), GFP_KERNEL);
> +	if (!phy_roothub)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&phy_roothub->list);
>  
>  	for (i = 0; i < num_phys; i++) {
>  		err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list);
>  		if (err)
> -			goto err_out;
> +			return ERR_PTR(err);
>  	}
>  
> +	return phy_roothub;
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_alloc);
> +
> +int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub)
> +{
> +	struct usb_phy_roothub *roothub_entry;
> +	struct list_head *head;
> +	int err;
> +
> +	if (!phy_roothub)
> +		return 0;
> +
>  	head = &phy_roothub->list;
>  
>  	list_for_each_entry(roothub_entry, head, list) {
> @@ -86,14 +88,13 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>  			goto err_exit_phys;
>  	}
>  
> -	return phy_roothub;
> +	return 0;
>  
>  err_exit_phys:
>  	list_for_each_entry_continue_reverse(roothub_entry, head, list)
>  		phy_exit(roothub_entry->phy);
>  
> -err_out:
> -	return ERR_PTR(err);
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
>  
> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> index 6fde59bfbff8..eb31253201ad 100644
> --- a/drivers/usb/core/phy.h
> +++ b/drivers/usb/core/phy.h
> @@ -1,6 +1,8 @@
>  struct usb_phy_roothub;
>  
> -struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev);
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);
> +
> +int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub);
>  int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
>  
>  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
> 

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[RFC usb-next v3 1/2] usb: core: split usb_phy_roothub_{init,alloc}

[Roger Quadros]

On 26/03/18 23:38, Martin Blumenstingl wrote:
> Before this patch usb_phy_roothub_init served two purposes (from a
> caller's point of view - like hcd.c):
> - parsing the PHYs and allocating the list entries
> - calling phy_init on each list entry
> 
> While this worked so far it has one disadvantage: if we need to call
> phy_init for each PHY instance then the existing code cannot be re-used.
> Solve this by splitting off usb_phy_roothub_alloc which only parses the
> PHYs and allocates the list entries.
> usb_phy_roothub_init then gets a struct usb_phy_roothub and only calls
> phy_init on each PHY instance (along with the corresponding cleanup if
> that failed somewhere).
> 
> This is a preparation step for adding proper suspend support for some
> hardware that requires phy_exit to be called during suspend and phy_init
> to be called during resume.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

I don't think we need RFC in subject.

Reviewed-by: Roger Quadros <rogerq@ti.com>

> ---
>  drivers/usb/core/hcd.c | 10 +++++++---
>  drivers/usb/core/phy.c | 53 +++++++++++++++++++++++++-------------------------
>  drivers/usb/core/phy.h |  4 +++-
>  3 files changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 777036ae6367..15b0418e3b6a 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2758,12 +2758,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	}
>  
>  	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
> -		hcd->phy_roothub = usb_phy_roothub_init(hcd->self.sysdev);
> +		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
>  		if (IS_ERR(hcd->phy_roothub)) {
>  			retval = PTR_ERR(hcd->phy_roothub);
> -			goto err_phy_roothub_init;
> +			goto err_phy_roothub_alloc;
>  		}
>  
> +		retval = usb_phy_roothub_init(hcd->phy_roothub);
> +		if (retval)
> +			goto err_phy_roothub_alloc;
> +
>  		retval = usb_phy_roothub_power_on(hcd->phy_roothub);
>  		if (retval)
>  			goto err_usb_phy_roothub_power_on;
> @@ -2936,7 +2940,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	usb_phy_roothub_power_off(hcd->phy_roothub);
>  err_usb_phy_roothub_power_on:
>  	usb_phy_roothub_exit(hcd->phy_roothub);
> -err_phy_roothub_init:
> +err_phy_roothub_alloc:
>  	if (hcd->remove_phy && hcd->usb_phy) {
>  		usb_phy_shutdown(hcd->usb_phy);
>  		usb_put_phy(hcd->usb_phy);
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index f19aaa3c899c..44f008cda7a8 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -19,19 +19,6 @@ struct usb_phy_roothub {
>  	struct list_head	list;
>  };
>  
> -static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
> -{
> -	struct usb_phy_roothub *roothub_entry;
> -
> -	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> -	if (!roothub_entry)
> -		return ERR_PTR(-ENOMEM);
> -
> -	INIT_LIST_HEAD(&roothub_entry->list);
> -
> -	return roothub_entry;
> -}
> -
>  static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  				   struct list_head *list)
>  {
> @@ -45,9 +32,11 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  			return PTR_ERR(phy);
>  	}
>  
> -	roothub_entry = usb_phy_roothub_alloc(dev);
> -	if (IS_ERR(roothub_entry))
> -		return PTR_ERR(roothub_entry);
> +	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> +	if (!roothub_entry)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&roothub_entry->list);
>  
>  	roothub_entry->phy = phy;
>  
> @@ -56,11 +45,9 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  	return 0;
>  }
>  
> -struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
>  {
>  	struct usb_phy_roothub *phy_roothub;
> -	struct usb_phy_roothub *roothub_entry;
> -	struct list_head *head;
>  	int i, num_phys, err;
>  
>  	num_phys = of_count_phandle_with_args(dev->of_node, "phys",
> @@ -68,16 +55,31 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>  	if (num_phys <= 0)
>  		return NULL;
>  
> -	phy_roothub = usb_phy_roothub_alloc(dev);
> -	if (IS_ERR(phy_roothub))
> -		return phy_roothub;
> +	phy_roothub = devm_kzalloc(dev, sizeof(*phy_roothub), GFP_KERNEL);
> +	if (!phy_roothub)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&phy_roothub->list);
>  
>  	for (i = 0; i < num_phys; i++) {
>  		err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list);
>  		if (err)
> -			goto err_out;
> +			return ERR_PTR(err);
>  	}
>  
> +	return phy_roothub;
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_alloc);
> +
> +int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub)
> +{
> +	struct usb_phy_roothub *roothub_entry;
> +	struct list_head *head;
> +	int err;
> +
> +	if (!phy_roothub)
> +		return 0;
> +
>  	head = &phy_roothub->list;
>  
>  	list_for_each_entry(roothub_entry, head, list) {
> @@ -86,14 +88,13 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>  			goto err_exit_phys;
>  	}
>  
> -	return phy_roothub;
> +	return 0;
>  
>  err_exit_phys:
>  	list_for_each_entry_continue_reverse(roothub_entry, head, list)
>  		phy_exit(roothub_entry->phy);
>  
> -err_out:
> -	return ERR_PTR(err);
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
>  
> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> index 6fde59bfbff8..eb31253201ad 100644
> --- a/drivers/usb/core/phy.h
> +++ b/drivers/usb/core/phy.h
> @@ -1,6 +1,8 @@
>  struct usb_phy_roothub;
>  
> -struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev);
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);
> +
> +int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub);
>  int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
>  
>  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
> 

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[RFC,usb-next,v3,2/2] usb: core: use phy_exit during suspend if wake up is not supported

[Roger Quadros]

On 26/03/18 23:38, Martin Blumenstingl wrote:
> If the USB controller can wake up the system (which is the case for
> example with the Mediatek USB3 IP) then we must not call phy_exit during
> suspend to ensure that the USB controller doesn't have to re-enumerate
> the devices during resume.
> However, if the USB controller cannot wake up the system (which is the
> case for example on various TI platforms using a dwc3 controller) then
> we must call phy_exit during suspend. Otherwise the PHY driver keeps the
> clocks enabled, which prevents the system from reaching the lowest power
> levels in the suspend state.
> 
> Solve this by introducing two new functions in the PHY wrapper which are
> dedicated to the suspend and resume handling.
> If the controller can wake up the system the new usb_phy_roothub_suspend
> function will simply call usb_phy_roothub_power_off. However, if wake up
> is not supported by the controller it will also call
> usb_phy_roothub_exit.
> The also new usb_phy_roothub_resume function takes care of calling
> usb_phy_roothub_init (if the controller can't wake up the system) in
> addition to usb_phy_roothub_power_on.
> 
> Fixes: 07dbff0ddbd86c ("usb: core: add a wrapper for the USB PHYs on the HCD")
> Fixes: 178a0bce05cbc1 ("usb: core: hcd: integrate the PHY wrapper into the HCD core")
> Reported-by: Roger Quadros <rogerq@ti.com>
> Suggested-by: Roger Quadros <rogerq@ti.com>
> Suggested-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Reviewed-by: Roger Quadros <rogerq@ti.com>

> ---
>  drivers/usb/core/hcd.c |  8 +++++---
>  drivers/usb/core/phy.c | 35 +++++++++++++++++++++++++++++++++++
>  drivers/usb/core/phy.h |  5 +++++
>  3 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 15b0418e3b6a..78bae4ecd68b 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2262,7 +2262,8 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
>  		hcd->state = HC_STATE_SUSPENDED;
>  
>  		if (!PMSG_IS_AUTO(msg))
> -			usb_phy_roothub_power_off(hcd->phy_roothub);
> +			usb_phy_roothub_suspend(hcd->self.sysdev,
> +						hcd->phy_roothub);
>  
>  		/* Did we race with a root-hub wakeup event? */
>  		if (rhdev->do_remote_wakeup) {
> @@ -2302,7 +2303,8 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  	}
>  
>  	if (!PMSG_IS_AUTO(msg)) {
> -		status = usb_phy_roothub_power_on(hcd->phy_roothub);
> +		status = usb_phy_roothub_resume(hcd->self.sysdev,
> +						hcd->phy_roothub);
>  		if (status)
>  			return status;
>  	}
> @@ -2344,7 +2346,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  		}
>  	} else {
>  		hcd->state = old_state;
> -		usb_phy_roothub_power_off(hcd->phy_roothub);
> +		usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
>  		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
>  				"resume", status);
>  		if (status != -ESHUTDOWN)
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index 44f008cda7a8..a39d9bb26a4f 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -157,3 +157,38 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
>  		phy_power_off(roothub_entry->phy);
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
> +
> +int usb_phy_roothub_suspend(struct device *controller_dev,
> +			    struct usb_phy_roothub *phy_roothub)
> +{
> +	usb_phy_roothub_power_off(phy_roothub);
> +
> +	/* keep the PHYs initialized so the device can wake up the system */
> +	if (device_may_wakeup(controller_dev))
> +		return 0;
> +
> +	return usb_phy_roothub_exit(phy_roothub);
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_suspend);
> +
> +int usb_phy_roothub_resume(struct device *controller_dev,
> +			   struct usb_phy_roothub *phy_roothub)
> +{
> +	int err;
> +
> +	/* if the device can't wake up the system _exit was called */
> +	if (!device_may_wakeup(controller_dev)) {
> +		err = usb_phy_roothub_init(phy_roothub);
> +		if (err)
> +			return err;
> +	}
> +
> +	err = usb_phy_roothub_power_on(phy_roothub);
> +
> +	/* undo _init if _power_on failed */
> +	if (err && !device_may_wakeup(controller_dev))
> +		usb_phy_roothub_exit(phy_roothub);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_resume);
> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> index eb31253201ad..605555901d44 100644
> --- a/drivers/usb/core/phy.h
> +++ b/drivers/usb/core/phy.h
> @@ -7,3 +7,8 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
>  
>  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
>  void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
> +
> +int usb_phy_roothub_suspend(struct device *controller_dev,
> +			    struct usb_phy_roothub *phy_roothub);
> +int usb_phy_roothub_resume(struct device *controller_dev,
> +			   struct usb_phy_roothub *phy_roothub);
>

Re: [RFC usb-next v3 2/2] usb: core: use phy_exit during suspend if wake up is not supported

[Roger Quadros]

On 26/03/18 23:38, Martin Blumenstingl wrote:
> If the USB controller can wake up the system (which is the case for
> example with the Mediatek USB3 IP) then we must not call phy_exit during
> suspend to ensure that the USB controller doesn't have to re-enumerate
> the devices during resume.
> However, if the USB controller cannot wake up the system (which is the
> case for example on various TI platforms using a dwc3 controller) then
> we must call phy_exit during suspend. Otherwise the PHY driver keeps the
> clocks enabled, which prevents the system from reaching the lowest power
> levels in the suspend state.
> 
> Solve this by introducing two new functions in the PHY wrapper which are
> dedicated to the suspend and resume handling.
> If the controller can wake up the system the new usb_phy_roothub_suspend
> function will simply call usb_phy_roothub_power_off. However, if wake up
> is not supported by the controller it will also call
> usb_phy_roothub_exit.
> The also new usb_phy_roothub_resume function takes care of calling
> usb_phy_roothub_init (if the controller can't wake up the system) in
> addition to usb_phy_roothub_power_on.
> 
> Fixes: 07dbff0ddbd86c ("usb: core: add a wrapper for the USB PHYs on the HCD")
> Fixes: 178a0bce05cbc1 ("usb: core: hcd: integrate the PHY wrapper into the HCD core")
> Reported-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
> Suggested-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
> Suggested-by: Chunfeng Yun <chunfeng.yun-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>

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

> ---
>  drivers/usb/core/hcd.c |  8 +++++---
>  drivers/usb/core/phy.c | 35 +++++++++++++++++++++++++++++++++++
>  drivers/usb/core/phy.h |  5 +++++
>  3 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 15b0418e3b6a..78bae4ecd68b 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2262,7 +2262,8 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
>  		hcd->state = HC_STATE_SUSPENDED;
>  
>  		if (!PMSG_IS_AUTO(msg))
> -			usb_phy_roothub_power_off(hcd->phy_roothub);
> +			usb_phy_roothub_suspend(hcd->self.sysdev,
> +						hcd->phy_roothub);
>  
>  		/* Did we race with a root-hub wakeup event? */
>  		if (rhdev->do_remote_wakeup) {
> @@ -2302,7 +2303,8 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  	}
>  
>  	if (!PMSG_IS_AUTO(msg)) {
> -		status = usb_phy_roothub_power_on(hcd->phy_roothub);
> +		status = usb_phy_roothub_resume(hcd->self.sysdev,
> +						hcd->phy_roothub);
>  		if (status)
>  			return status;
>  	}
> @@ -2344,7 +2346,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  		}
>  	} else {
>  		hcd->state = old_state;
> -		usb_phy_roothub_power_off(hcd->phy_roothub);
> +		usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
>  		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
>  				"resume", status);
>  		if (status != -ESHUTDOWN)
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index 44f008cda7a8..a39d9bb26a4f 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -157,3 +157,38 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
>  		phy_power_off(roothub_entry->phy);
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
> +
> +int usb_phy_roothub_suspend(struct device *controller_dev,
> +			    struct usb_phy_roothub *phy_roothub)
> +{
> +	usb_phy_roothub_power_off(phy_roothub);
> +
> +	/* keep the PHYs initialized so the device can wake up the system */
> +	if (device_may_wakeup(controller_dev))
> +		return 0;
> +
> +	return usb_phy_roothub_exit(phy_roothub);
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_suspend);
> +
> +int usb_phy_roothub_resume(struct device *controller_dev,
> +			   struct usb_phy_roothub *phy_roothub)
> +{
> +	int err;
> +
> +	/* if the device can't wake up the system _exit was called */
> +	if (!device_may_wakeup(controller_dev)) {
> +		err = usb_phy_roothub_init(phy_roothub);
> +		if (err)
> +			return err;
> +	}
> +
> +	err = usb_phy_roothub_power_on(phy_roothub);
> +
> +	/* undo _init if _power_on failed */
> +	if (err && !device_may_wakeup(controller_dev))
> +		usb_phy_roothub_exit(phy_roothub);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_resume);
> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> index eb31253201ad..605555901d44 100644
> --- a/drivers/usb/core/phy.h
> +++ b/drivers/usb/core/phy.h
> @@ -7,3 +7,8 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
>  
>  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
>  void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
> +
> +int usb_phy_roothub_suspend(struct device *controller_dev,
> +			    struct usb_phy_roothub *phy_roothub);
> +int usb_phy_roothub_resume(struct device *controller_dev,
> +			   struct usb_phy_roothub *phy_roothub);
> 

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[RFC usb-next v3 2/2] usb: core: use phy_exit during suspend if wake up is not supported

[Roger Quadros]

On 26/03/18 23:38, Martin Blumenstingl wrote:
> If the USB controller can wake up the system (which is the case for
> example with the Mediatek USB3 IP) then we must not call phy_exit during
> suspend to ensure that the USB controller doesn't have to re-enumerate
> the devices during resume.
> However, if the USB controller cannot wake up the system (which is the
> case for example on various TI platforms using a dwc3 controller) then
> we must call phy_exit during suspend. Otherwise the PHY driver keeps the
> clocks enabled, which prevents the system from reaching the lowest power
> levels in the suspend state.
> 
> Solve this by introducing two new functions in the PHY wrapper which are
> dedicated to the suspend and resume handling.
> If the controller can wake up the system the new usb_phy_roothub_suspend
> function will simply call usb_phy_roothub_power_off. However, if wake up
> is not supported by the controller it will also call
> usb_phy_roothub_exit.
> The also new usb_phy_roothub_resume function takes care of calling
> usb_phy_roothub_init (if the controller can't wake up the system) in
> addition to usb_phy_roothub_power_on.
> 
> Fixes: 07dbff0ddbd86c ("usb: core: add a wrapper for the USB PHYs on the HCD")
> Fixes: 178a0bce05cbc1 ("usb: core: hcd: integrate the PHY wrapper into the HCD core")
> Reported-by: Roger Quadros <rogerq@ti.com>
> Suggested-by: Roger Quadros <rogerq@ti.com>
> Suggested-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Reviewed-by: Roger Quadros <rogerq@ti.com>

> ---
>  drivers/usb/core/hcd.c |  8 +++++---
>  drivers/usb/core/phy.c | 35 +++++++++++++++++++++++++++++++++++
>  drivers/usb/core/phy.h |  5 +++++
>  3 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 15b0418e3b6a..78bae4ecd68b 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2262,7 +2262,8 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
>  		hcd->state = HC_STATE_SUSPENDED;
>  
>  		if (!PMSG_IS_AUTO(msg))
> -			usb_phy_roothub_power_off(hcd->phy_roothub);
> +			usb_phy_roothub_suspend(hcd->self.sysdev,
> +						hcd->phy_roothub);
>  
>  		/* Did we race with a root-hub wakeup event? */
>  		if (rhdev->do_remote_wakeup) {
> @@ -2302,7 +2303,8 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  	}
>  
>  	if (!PMSG_IS_AUTO(msg)) {
> -		status = usb_phy_roothub_power_on(hcd->phy_roothub);
> +		status = usb_phy_roothub_resume(hcd->self.sysdev,
> +						hcd->phy_roothub);
>  		if (status)
>  			return status;
>  	}
> @@ -2344,7 +2346,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  		}
>  	} else {
>  		hcd->state = old_state;
> -		usb_phy_roothub_power_off(hcd->phy_roothub);
> +		usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
>  		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
>  				"resume", status);
>  		if (status != -ESHUTDOWN)
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index 44f008cda7a8..a39d9bb26a4f 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -157,3 +157,38 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
>  		phy_power_off(roothub_entry->phy);
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
> +
> +int usb_phy_roothub_suspend(struct device *controller_dev,
> +			    struct usb_phy_roothub *phy_roothub)
> +{
> +	usb_phy_roothub_power_off(phy_roothub);
> +
> +	/* keep the PHYs initialized so the device can wake up the system */
> +	if (device_may_wakeup(controller_dev))
> +		return 0;
> +
> +	return usb_phy_roothub_exit(phy_roothub);
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_suspend);
> +
> +int usb_phy_roothub_resume(struct device *controller_dev,
> +			   struct usb_phy_roothub *phy_roothub)
> +{
> +	int err;
> +
> +	/* if the device can't wake up the system _exit was called */
> +	if (!device_may_wakeup(controller_dev)) {
> +		err = usb_phy_roothub_init(phy_roothub);
> +		if (err)
> +			return err;
> +	}
> +
> +	err = usb_phy_roothub_power_on(phy_roothub);
> +
> +	/* undo _init if _power_on failed */
> +	if (err && !device_may_wakeup(controller_dev))
> +		usb_phy_roothub_exit(phy_roothub);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_resume);
> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> index eb31253201ad..605555901d44 100644
> --- a/drivers/usb/core/phy.h
> +++ b/drivers/usb/core/phy.h
> @@ -7,3 +7,8 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
>  
>  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
>  void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
> +
> +int usb_phy_roothub_suspend(struct device *controller_dev,
> +			    struct usb_phy_roothub *phy_roothub);
> +int usb_phy_roothub_resume(struct device *controller_dev,
> +			   struct usb_phy_roothub *phy_roothub);
> 

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[RFC,usb-next,v3,1/2] usb: core: split usb_phy_roothub_{init,alloc}

[Greg Kroah-Hartman]

On Tue, Mar 27, 2018 at 11:24:08AM +0300, Roger Quadros wrote:
> On 26/03/18 23:38, Martin Blumenstingl wrote:
> > Before this patch usb_phy_roothub_init served two purposes (from a
> > caller's point of view - like hcd.c):
> > - parsing the PHYs and allocating the list entries
> > - calling phy_init on each list entry
> > 
> > While this worked so far it has one disadvantage: if we need to call
> > phy_init for each PHY instance then the existing code cannot be re-used.
> > Solve this by splitting off usb_phy_roothub_alloc which only parses the
> > PHYs and allocates the list entries.
> > usb_phy_roothub_init then gets a struct usb_phy_roothub and only calls
> > phy_init on each PHY instance (along with the corresponding cleanup if
> > that failed somewhere).
> > 
> > This is a preparation step for adding proper suspend support for some
> > hardware that requires phy_exit to be called during suspend and phy_init
> > to be called during resume.
> > 
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> I don't think we need RFC in subject.

As I don't apply series with RFC in the subject, that might be a good
idea :)
---
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

Re: [RFC usb-next v3 1/2] usb: core: split usb_phy_roothub_{init,alloc}

[Greg KH]

On Tue, Mar 27, 2018 at 11:24:08AM +0300, Roger Quadros wrote:
> On 26/03/18 23:38, Martin Blumenstingl wrote:
> > Before this patch usb_phy_roothub_init served two purposes (from a
> > caller's point of view - like hcd.c):
> > - parsing the PHYs and allocating the list entries
> > - calling phy_init on each list entry
> > 
> > While this worked so far it has one disadvantage: if we need to call
> > phy_init for each PHY instance then the existing code cannot be re-used.
> > Solve this by splitting off usb_phy_roothub_alloc which only parses the
> > PHYs and allocates the list entries.
> > usb_phy_roothub_init then gets a struct usb_phy_roothub and only calls
> > phy_init on each PHY instance (along with the corresponding cleanup if
> > that failed somewhere).
> > 
> > This is a preparation step for adding proper suspend support for some
> > hardware that requires phy_exit to be called during suspend and phy_init
> > to be called during resume.
> > 
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> 
> I don't think we need RFC in subject.

As I don't apply series with RFC in the subject, that might be a good
idea :)

[RFC usb-next v3 1/2] usb: core: split usb_phy_roothub_{init,alloc}

[Greg KH]

On Tue, Mar 27, 2018 at 11:24:08AM +0300, Roger Quadros wrote:
> On 26/03/18 23:38, Martin Blumenstingl wrote:
> > Before this patch usb_phy_roothub_init served two purposes (from a
> > caller's point of view - like hcd.c):
> > - parsing the PHYs and allocating the list entries
> > - calling phy_init on each list entry
> > 
> > While this worked so far it has one disadvantage: if we need to call
> > phy_init for each PHY instance then the existing code cannot be re-used.
> > Solve this by splitting off usb_phy_roothub_alloc which only parses the
> > PHYs and allocates the list entries.
> > usb_phy_roothub_init then gets a struct usb_phy_roothub and only calls
> > phy_init on each PHY instance (along with the corresponding cleanup if
> > that failed somewhere).
> > 
> > This is a preparation step for adding proper suspend support for some
> > hardware that requires phy_exit to be called during suspend and phy_init
> > to be called during resume.
> > 
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> I don't think we need RFC in subject.

As I don't apply series with RFC in the subject, that might be a good
idea :)