[RFC v4] usb: phy: Hold wakeupsource when USB is enumerated in peripheral mode

[RFC v4] usb: phy: Hold wakeupsource when USB is enumerated in peripheral mode

[Kiran Kumar Raparthy]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 8216 bytes --]

From: Todd Poynor <toddpoynor@google.com>

usb: phy: Hold wakeupsource when USB is enumerated in peripheral mode

Purpose of this is to prevent the system to enter into suspend state from USB
peripheral traffic by hodling a wakeupsource when USB is connected and
enumerated in peripheral mode(say adb).

Temporarily hold a timed wakeup source on USB disconnect events, to allow
the rest of the system to react to the USB disconnection (dropping host
sessions, updating charger status, etc.) prior to re-allowing suspend.

Cc: Felipe Balbi <balbi@ti.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org
Cc: linux-usb@vger.kernel.org
Cc: Android Kernel Team <kernel-team@android.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Arve Hjønnevåg <arve@android.com>
Cc: Benoit Goby <benoit@android.com>
Signed-off-by: Todd Poynor <toddpoynor@google.com>
[kiran: Added context to commit message, squished build fixes
from Benoit Goby and Arve Hjønnevåg, changed wakelocks usage
to wakeupsource, merged Todd's refactoring logic and simplified
the structures and code and addressed community feedback]
Signed-off-by: Kiran Raparthy <kiran.kumar@linaro.org>
---
v4:
* Temporarily hold wakeupsource patch integrated into main patch.
* As per feedback,dropped "enabled" module parameter.
* Introduced otgws_otg_usb3_notifications function to handle event
  notifications from usb3 phy.
* Handled wakeupsource initialization,spinlock,registration of notifier block
  per-PHY.
* Updated usb_phy structure.

v3:
* As per the feedback,no global phy pointer used.
* called the one-liner wakeupsource handling calls
  directly instead of indirect functions implemented in v2.
* Removed indirect function get_phy_hook and used usb_get_phy
  to get the phy handle..

v2:
* wakeupsource handling implemeted per-PHY
* Implemented wakeupsource handling calls in phy
* included Todd's refactoring logic.

v1:
* changed to "disabled by default" from "enable by default".
* Kconfig help text modified
* Included better commit text
* otgws_nb moved to otg_wakeupsource_init function
* Introduced get_phy_hook to handle otgws_xceiv per-PHY

RFC:
* Included build fix from Benoit Goby and Arve Hjønnevåg
* Removed lock->held field in driver as this mechanism is
  provided in wakeupsource driver.
* wakelock(wl) terminology replaced with wakeup_source(ws).

 drivers/usb/phy/Kconfig            |   8 +++
 drivers/usb/phy/Makefile           |   1 +
 drivers/usb/phy/otg-wakeupsource.c | 134 +++++++++++++++++++++++++++++++++++++
 include/linux/usb/phy.h            |   8 +++
 4 files changed, 151 insertions(+)
 create mode 100644 drivers/usb/phy/otg-wakeupsource.c

diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
index e253fa0..d9ddd85 100644
--- a/drivers/usb/phy/Kconfig
+++ b/drivers/usb/phy/Kconfig
@@ -6,6 +6,14 @@ menu "USB Physical Layer drivers"
 config USB_PHY
 	def_bool n
 
+config USB_OTG_WAKEUPSOURCE
+	bool "Hold wakeupsource when USB is enumerated in peripheral mode"
+	depends on PM_SLEEP
+	select USB_PHY
+	help
+	  Prevent the system going into automatic suspend while
+	  it is attached as a USB peripheral by holding a wakeupsource.
+
 #
 # USB Transceiver Drivers
 #
diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
index 24a9133..ca2fbaf 100644
--- a/drivers/usb/phy/Makefile
+++ b/drivers/usb/phy/Makefile
@@ -3,6 +3,7 @@
 #
 obj-$(CONFIG_USB_PHY)			+= phy.o
 obj-$(CONFIG_OF)			+= of.o
+obj-$(CONFIG_USB_OTG_WAKEUPSOURCE)		+= otg-wakeupsource.o
 
 # transceiver drivers, keep the list sorted
 
diff --git a/drivers/usb/phy/otg-wakeupsource.c b/drivers/usb/phy/otg-wakeupsource.c
new file mode 100644
index 0000000..00d3359
--- /dev/null
+++ b/drivers/usb/phy/otg-wakeupsource.c
@@ -0,0 +1,134 @@
+/*
+ * otg-wakeupsource.c
+ *
+ * Copyright (C) 2011 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/pm_wakeup.h>
+#include <linux/spinlock.h>
+#include <linux/usb/otg.h>
+
+static void otgws_handle_event(struct usb_phy *otgws_xceiv, unsigned long event)
+{
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&otgws_xceiv->otgws_slock, irqflags);
+
+	switch (event) {
+	case USB_EVENT_VBUS:
+	case USB_EVENT_ENUMERATED:
+		__pm_stay_awake(&otgws_xceiv->wsource);
+		break;
+
+	case USB_EVENT_NONE:
+	case USB_EVENT_ID:
+	case USB_EVENT_CHARGER:
+		__pm_wakeup_event(&otgws_xceiv->wsource,
+				msecs_to_jiffies(TEMPORARY_HOLD_TIME));
+		break;
+
+	default:
+		break;
+	}
+
+	spin_unlock_irqrestore(&otgws_xceiv->otgws_slock, irqflags);
+}
+
+static int otgws_otg_usb2_notifications(struct notifier_block *nb,
+				unsigned long event, void *unused)
+{
+	static struct usb_phy *otgws_xceiv;
+
+	otgws_xceiv = usb_get_phy(USB_PHY_TYPE_USB2);
+
+	if (IS_ERR(otgws_xceiv)) {
+		pr_err("%s: No OTG transceiver found\n", __func__);
+		return PTR_ERR(otgws_xceiv);
+	}
+
+	otgws_handle_event(otgws_xceiv, event);
+
+	return NOTIFY_OK;
+}
+
+static int otgws_otg_usb3_notifications(struct notifier_block *nb,
+				unsigned long event, void *unused)
+{
+	static struct usb_phy *otgws_xceiv;
+
+	otgws_xceiv = usb_get_phy(USB_PHY_TYPE_USB3);
+
+	if (IS_ERR(otgws_xceiv)) {
+		pr_err("%s: No OTG transceiver found\n", __func__);
+		return PTR_ERR(otgws_xceiv);
+	}
+
+	otgws_handle_event(otgws_xceiv, event);
+
+	return NOTIFY_OK;
+}
+
+static int otg_wakeupsource_init(void)
+{
+	int ret_usb2;
+	int ret_usb3;
+	char wsource_name_usb2[40];
+	char wsource_name_usb3[40];
+	static struct usb_phy *otgws_xceiv_usb2;
+	static struct usb_phy *otgws_xceiv_usb3;
+
+	otgws_xceiv_usb2 = usb_get_phy(USB_PHY_TYPE_USB2);
+	otgws_xceiv_usb3 = usb_get_phy(USB_PHY_TYPE_USB3);
+
+	if (IS_ERR(otgws_xceiv_usb2) && IS_ERR(otgws_xceiv_usb3)) {
+		pr_err("%s: No OTG transceiver found\n", __func__);
+		return PTR_ERR(otgws_xceiv_usb2);
+	}
+
+	spin_lock_init(&otgws_xceiv_usb2->otgws_slock);
+	spin_lock_init(&otgws_xceiv_usb3->otgws_slock);
+
+	snprintf(wsource_name_usb2, sizeof(wsource_name_usb2), "vbus-%s",
+		dev_name(otgws_xceiv_usb2->dev));
+	wakeup_source_init(&otgws_xceiv_usb2->wsource, wsource_name_usb2);
+
+	snprintf(wsource_name_usb3, sizeof(wsource_name_usb3), "vbus-%s",
+		dev_name(otgws_xceiv_usb3->dev));
+	wakeup_source_init(&otgws_xceiv_usb3->wsource, wsource_name_usb3);
+
+	otgws_xceiv_usb2->otgws_nb.notifier_call = otgws_otg_usb2_notifications;
+	ret_usb2 = usb_register_notifier(otgws_xceiv_usb2,
+					&otgws_xceiv_usb2->otgws_nb);
+
+	otgws_xceiv_usb3->otgws_nb.notifier_call = otgws_otg_usb3_notifications;
+	ret_usb3 = usb_register_notifier(otgws_xceiv_usb3,
+					&otgws_xceiv_usb3->otgws_nb);
+
+	if (ret_usb2 && ret_usb3) {
+		pr_err("%s: usb_register_notifier on transceiver failed\n",
+			 __func__);
+		wakeup_source_trash(&otgws_xceiv_usb2->wsource);
+		wakeup_source_trash(&otgws_xceiv_usb3->wsource);
+		otgws_xceiv_usb2 = NULL;
+		otgws_xceiv_usb3 = NULL;
+		return ret_usb2 | ret_usb3;
+	}
+
+	return 0;
+}
+
+late_initcall(otg_wakeupsource_init);
diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
index 353053a..dd64e2e 100644
--- a/include/linux/usb/phy.h
+++ b/include/linux/usb/phy.h
@@ -12,6 +12,8 @@
 #include <linux/notifier.h>
 #include <linux/usb.h>
 
+#define TEMPORARY_HOLD_TIME    2000
+
 enum usb_phy_interface {
 	USBPHY_INTERFACE_MODE_UNKNOWN,
 	USBPHY_INTERFACE_MODE_UTMI,
@@ -88,6 +90,12 @@ struct usb_phy {
 
 	/* for notification of usb_phy_events */
 	struct atomic_notifier_head	notifier;
+	struct notifier_block	otgws_nb;
+
+	/* wakeup source */
+	struct wakeup_source	wsource;
+
+	spinlock_t		otgws_slock;
 
 	/* to pass extra port status to the root hub */
 	u16			port_status;
-- 
1.8.2.1

Re: [RFC v4] usb: phy: Hold wakeupsource when USB is enumerated in peripheral mode

[Felipe Balbi]

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

Hi,

On Tue, Oct 07, 2014 at 02:45:44PM +0530, Kiran Kumar Raparthy wrote:
> diff --git a/drivers/usb/phy/otg-wakeupsource.c b/drivers/usb/phy/otg-wakeupsource.c
> new file mode 100644
> index 0000000..00d3359
> --- /dev/null
> +++ b/drivers/usb/phy/otg-wakeupsource.c
> @@ -0,0 +1,134 @@
> +/*
> + * otg-wakeupsource.c
> + *
> + * Copyright (C) 2011 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +#include <linux/pm_wakeup.h>
> +#include <linux/spinlock.h>
> +#include <linux/usb/otg.h>
> +
> +static void otgws_handle_event(struct usb_phy *otgws_xceiv, unsigned long event)
> +{
> +	unsigned long irqflags;
> +
> +	spin_lock_irqsave(&otgws_xceiv->otgws_slock, irqflags);
> +
> +	switch (event) {
> +	case USB_EVENT_VBUS:

Looks like VBUS should be temporary too.

> +	case USB_EVENT_ENUMERATED:
> +		__pm_stay_awake(&otgws_xceiv->wsource);
> +		break;
> +
> +	case USB_EVENT_NONE:
> +	case USB_EVENT_ID:
> +	case USB_EVENT_CHARGER:
> +		__pm_wakeup_event(&otgws_xceiv->wsource,
> +				msecs_to_jiffies(TEMPORARY_HOLD_TIME));
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	spin_unlock_irqrestore(&otgws_xceiv->otgws_slock, irqflags);
> +}
> +
> +static int otgws_otg_usb2_notifications(struct notifier_block *nb,
> +				unsigned long event, void *unused)
> +{
> +	static struct usb_phy *otgws_xceiv;
> +
> +	otgws_xceiv = usb_get_phy(USB_PHY_TYPE_USB2);
> +
> +	if (IS_ERR(otgws_xceiv)) {
> +		pr_err("%s: No OTG transceiver found\n", __func__);
> +		return PTR_ERR(otgws_xceiv);
> +	}
> +
> +	otgws_handle_event(otgws_xceiv, event);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int otgws_otg_usb3_notifications(struct notifier_block *nb,
> +				unsigned long event, void *unused)
> +{
> +	static struct usb_phy *otgws_xceiv;
> +
> +	otgws_xceiv = usb_get_phy(USB_PHY_TYPE_USB3);
> +
> +	if (IS_ERR(otgws_xceiv)) {
> +		pr_err("%s: No OTG transceiver found\n", __func__);
> +		return PTR_ERR(otgws_xceiv);
> +	}
> +
> +	otgws_handle_event(otgws_xceiv, event);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int otg_wakeupsource_init(void)
> +{
> +	int ret_usb2;
> +	int ret_usb3;
> +	char wsource_name_usb2[40];
> +	char wsource_name_usb3[40];
> +	static struct usb_phy *otgws_xceiv_usb2;
> +	static struct usb_phy *otgws_xceiv_usb3;
> +
> +	otgws_xceiv_usb2 = usb_get_phy(USB_PHY_TYPE_USB2);
> +	otgws_xceiv_usb3 = usb_get_phy(USB_PHY_TYPE_USB3);
> +
> +	if (IS_ERR(otgws_xceiv_usb2) && IS_ERR(otgws_xceiv_usb3)) {
> +		pr_err("%s: No OTG transceiver found\n", __func__);
> +		return PTR_ERR(otgws_xceiv_usb2);
> +	}
> +
> +	spin_lock_init(&otgws_xceiv_usb2->otgws_slock);
> +	spin_lock_init(&otgws_xceiv_usb3->otgws_slock);
> +
> +	snprintf(wsource_name_usb2, sizeof(wsource_name_usb2), "vbus-%s",
> +		dev_name(otgws_xceiv_usb2->dev));
> +	wakeup_source_init(&otgws_xceiv_usb2->wsource, wsource_name_usb2);
> +
> +	snprintf(wsource_name_usb3, sizeof(wsource_name_usb3), "vbus-%s",
> +		dev_name(otgws_xceiv_usb3->dev));
> +	wakeup_source_init(&otgws_xceiv_usb3->wsource, wsource_name_usb3);
> +
> +	otgws_xceiv_usb2->otgws_nb.notifier_call = otgws_otg_usb2_notifications;
> +	ret_usb2 = usb_register_notifier(otgws_xceiv_usb2,
> +					&otgws_xceiv_usb2->otgws_nb);
> +
> +	otgws_xceiv_usb3->otgws_nb.notifier_call = otgws_otg_usb3_notifications;
> +	ret_usb3 = usb_register_notifier(otgws_xceiv_usb3,
> +					&otgws_xceiv_usb3->otgws_nb);
> +
> +	if (ret_usb2 && ret_usb3) {
> +		pr_err("%s: usb_register_notifier on transceiver failed\n",
> +			 __func__);
> +		wakeup_source_trash(&otgws_xceiv_usb2->wsource);
> +		wakeup_source_trash(&otgws_xceiv_usb3->wsource);
> +		otgws_xceiv_usb2 = NULL;
> +		otgws_xceiv_usb3 = NULL;
> +		return ret_usb2 | ret_usb3;
> +	}
> +
> +	return 0;
> +}
> +
> +late_initcall(otg_wakeupsource_init);

you guys are really not getting what I mean. I asked for this to be
built into the core itself. This means that you shouldn't need to use
notifications nor should you need to call usb_get_phy(). You're part of
the PHY framework.

All this late_initcall() nonsense should go.

This code won't even work if we have more than one phy of the same type
(AM437x SoC, for example, has up to 4 instances of dwc3, so that's 4
USB2 PHYs), because you can't grab the PHY you want.

What you need is to:

1) make PHY notifiers generic (move all of that phy-core.c)
2) introduce usb_phy_set_event(phy, event) (which just sets the even to a
	phy->event member for now)
3) make all PHY drivers use usb_phy_set_event()
4) add the following to usb_phy_set_event()

	switch (event) {
	case USB_EVENT_ENUMERATED:
		pm_stay_awake(&otgws_xceiv->wsource);
		break;

	case USB_EVENT_NONE:
	case USB_EVENT_VBUS:
	case USB_EVENT_ID:
	case USB_EVENT_CHARGER:
		pm_wakeup_event(&otgws_xceiv->wsource,
				msecs_to_jiffies(TEMPORARY_HOLD_TIME));
		break;

	default:
		break;
	}

note that I'm calling locked versions of those functions so you can drop
the spinlock you added. But, dependending on when usb_phy_set_event() is
called, you might want to switch back to unlocked versions. In any case,
the new spinlock is unnecessary because you can either use
dev->power.lock or you're calling usb_phy_set_event() from and IRQ
handler.

> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
> index 353053a..dd64e2e 100644
> --- a/include/linux/usb/phy.h
> +++ b/include/linux/usb/phy.h
> @@ -12,6 +12,8 @@
>  #include <linux/notifier.h>
>  #include <linux/usb.h>
>  
> +#define TEMPORARY_HOLD_TIME    2000
> +
>  enum usb_phy_interface {
>  	USBPHY_INTERFACE_MODE_UNKNOWN,
>  	USBPHY_INTERFACE_MODE_UTMI,
> @@ -88,6 +90,12 @@ struct usb_phy {
>  
>  	/* for notification of usb_phy_events */
>  	struct atomic_notifier_head	notifier;
> +	struct notifier_block	otgws_nb;

drop this notifier block.

> +
> +	/* wakeup source */
> +	struct wakeup_source	wsource;

this is the only thing you need.

> +
> +	spinlock_t		otgws_slock;

drop this lock.

-- 
balbi

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

Re: [RFC v4] usb: phy: Hold wakeupsource when USB is enumerated in peripheral mode

[Kiran Raparthy]

Hi Felipe,
Thank you very much for taking time in reviewing the patch.
I will try to improve the patch as per your suggestions.
however,i have few queries which i wanted to understand from you.

On 7 October 2014 19:55, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Tue, Oct 07, 2014 at 02:45:44PM +0530, Kiran Kumar Raparthy wrote:
>> diff --git a/drivers/usb/phy/otg-wakeupsource.c b/drivers/usb/phy/otg-wakeupsource.c
>> new file mode 100644
>> index 0000000..00d3359
>> --- /dev/null
>> +++ b/drivers/usb/phy/otg-wakeupsource.c
>> @@ -0,0 +1,134 @@
>> +/*
>> + * otg-wakeupsource.c
>> + *
>> + * Copyright (C) 2011 Google, Inc.
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/notifier.h>
>> +#include <linux/pm_wakeup.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/usb/otg.h>
>> +
>> +static void otgws_handle_event(struct usb_phy *otgws_xceiv, unsigned long event)
>> +{
>> +     unsigned long irqflags;
>> +
>> +     spin_lock_irqsave(&otgws_xceiv->otgws_slock, irqflags);
>> +
>> +     switch (event) {
>> +     case USB_EVENT_VBUS:
>
> Looks like VBUS should be temporary too.
>
>> +     case USB_EVENT_ENUMERATED:
>> +             __pm_stay_awake(&otgws_xceiv->wsource);
>> +             break;
>> +
>> +     case USB_EVENT_NONE:
>> +     case USB_EVENT_ID:
>> +     case USB_EVENT_CHARGER:
>> +             __pm_wakeup_event(&otgws_xceiv->wsource,
>> +                             msecs_to_jiffies(TEMPORARY_HOLD_TIME));
>> +             break;
>> +
>> +     default:
>> +             break;
>> +     }
>> +
>> +     spin_unlock_irqrestore(&otgws_xceiv->otgws_slock, irqflags);
>> +}
>> +
>> +static int otgws_otg_usb2_notifications(struct notifier_block *nb,
>> +                             unsigned long event, void *unused)
>> +{
>> +     static struct usb_phy *otgws_xceiv;
>> +
>> +     otgws_xceiv = usb_get_phy(USB_PHY_TYPE_USB2);
>> +
>> +     if (IS_ERR(otgws_xceiv)) {
>> +             pr_err("%s: No OTG transceiver found\n", __func__);
>> +             return PTR_ERR(otgws_xceiv);
>> +     }
>> +
>> +     otgws_handle_event(otgws_xceiv, event);
>> +
>> +     return NOTIFY_OK;
>> +}
>> +
>> +static int otgws_otg_usb3_notifications(struct notifier_block *nb,
>> +                             unsigned long event, void *unused)
>> +{
>> +     static struct usb_phy *otgws_xceiv;
>> +
>> +     otgws_xceiv = usb_get_phy(USB_PHY_TYPE_USB3);
>> +
>> +     if (IS_ERR(otgws_xceiv)) {
>> +             pr_err("%s: No OTG transceiver found\n", __func__);
>> +             return PTR_ERR(otgws_xceiv);
>> +     }
>> +
>> +     otgws_handle_event(otgws_xceiv, event);
>> +
>> +     return NOTIFY_OK;
>> +}
>> +
>> +static int otg_wakeupsource_init(void)
>> +{
>> +     int ret_usb2;
>> +     int ret_usb3;
>> +     char wsource_name_usb2[40];
>> +     char wsource_name_usb3[40];
>> +     static struct usb_phy *otgws_xceiv_usb2;
>> +     static struct usb_phy *otgws_xceiv_usb3;
>> +
>> +     otgws_xceiv_usb2 = usb_get_phy(USB_PHY_TYPE_USB2);
>> +     otgws_xceiv_usb3 = usb_get_phy(USB_PHY_TYPE_USB3);
>> +
>> +     if (IS_ERR(otgws_xceiv_usb2) && IS_ERR(otgws_xceiv_usb3)) {
>> +             pr_err("%s: No OTG transceiver found\n", __func__);
>> +             return PTR_ERR(otgws_xceiv_usb2);
>> +     }
>> +
>> +     spin_lock_init(&otgws_xceiv_usb2->otgws_slock);
>> +     spin_lock_init(&otgws_xceiv_usb3->otgws_slock);
>> +
>> +     snprintf(wsource_name_usb2, sizeof(wsource_name_usb2), "vbus-%s",
>> +             dev_name(otgws_xceiv_usb2->dev));
>> +     wakeup_source_init(&otgws_xceiv_usb2->wsource, wsource_name_usb2);
>> +
>> +     snprintf(wsource_name_usb3, sizeof(wsource_name_usb3), "vbus-%s",
>> +             dev_name(otgws_xceiv_usb3->dev));
>> +     wakeup_source_init(&otgws_xceiv_usb3->wsource, wsource_name_usb3);
>> +
>> +     otgws_xceiv_usb2->otgws_nb.notifier_call = otgws_otg_usb2_notifications;
>> +     ret_usb2 = usb_register_notifier(otgws_xceiv_usb2,
>> +                                     &otgws_xceiv_usb2->otgws_nb);
>> +
>> +     otgws_xceiv_usb3->otgws_nb.notifier_call = otgws_otg_usb3_notifications;
>> +     ret_usb3 = usb_register_notifier(otgws_xceiv_usb3,
>> +                                     &otgws_xceiv_usb3->otgws_nb);
>> +
>> +     if (ret_usb2 && ret_usb3) {
>> +             pr_err("%s: usb_register_notifier on transceiver failed\n",
>> +                      __func__);
>> +             wakeup_source_trash(&otgws_xceiv_usb2->wsource);
>> +             wakeup_source_trash(&otgws_xceiv_usb3->wsource);
>> +             otgws_xceiv_usb2 = NULL;
>> +             otgws_xceiv_usb3 = NULL;
>> +             return ret_usb2 | ret_usb3;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +late_initcall(otg_wakeupsource_init);
>
> you guys are really not getting what I mean. I asked for this to be
> built into the core itself. This means that you shouldn't need to use
> notifications nor should you need to call usb_get_phy(). You're part of
> the PHY framework.
>
> All this late_initcall() nonsense should go.
>
> This code won't even work if we have more than one phy of the same type
> (AM437x SoC, for example, has up to 4 instances of dwc3, so that's 4
> USB2 PHYs), because you can't grab the PHY you want.

Apologies,I am new to usb sub system,so i missed this point before i
posted my patch,Thanks for the information.
>
> What you need is to:
>
> 1) make PHY notifiers generic (move all of that phy-core.c)
>From the above points,you mentioned that "if we built it into core,we
shouldn't need to use notifications"
and your first point here says that make phy notifiers generic in phy-core.c
can you help me understanding it better so that there wont be any
understanding gap.

> 2) introduce usb_phy_set_event(phy, event) (which just sets the even to a
>         phy->event member for now)
> 3) make all PHY drivers use usb_phy_set_event()
> 4) add the following to usb_phy_set_event()
>
>         switch (event) {
>         case USB_EVENT_ENUMERATED:
>                 pm_stay_awake(&otgws_xceiv->wsource);
>                 break;
>
>         case USB_EVENT_NONE:
>         case USB_EVENT_VBUS:
>         case USB_EVENT_ID:
>         case USB_EVENT_CHARGER:
>                 pm_wakeup_event(&otgws_xceiv->wsource,
>                                 msecs_to_jiffies(TEMPORARY_HOLD_TIME));
>                 break;
>
>         default:
>                 break;
>         }
>
Once the phy drivers receives per-PHY event notification(if we use
notifier,else "for any event") we can call usb_phy_set_event from phy
driver to hold the wakeup source.
Please correct me if my understanding is incorrect.

I have gone through some phy drivers in drivers/phy,since the each
driver implementation is different from others, i didn't get the best
place in  PHY driver
where we can trigger(use phy-core functionality) per-PHY notifier
registration. any pointers here?
Regards,
Kiran

> note that I'm calling locked versions of those functions so you can drop
> the spinlock you added. But, dependending on when usb_phy_set_event() is
> called, you might want to switch back to unlocked versions. In any case,
> the new spinlock is unnecessary because you can either use
> dev->power.lock or you're calling usb_phy_set_event() from and IRQ
> handler.
>
>> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
>> index 353053a..dd64e2e 100644
>> --- a/include/linux/usb/phy.h
>> +++ b/include/linux/usb/phy.h
>> @@ -12,6 +12,8 @@
>>  #include <linux/notifier.h>
>>  #include <linux/usb.h>
>>
>> +#define TEMPORARY_HOLD_TIME    2000
>> +
>>  enum usb_phy_interface {
>>       USBPHY_INTERFACE_MODE_UNKNOWN,
>>       USBPHY_INTERFACE_MODE_UTMI,
>> @@ -88,6 +90,12 @@ struct usb_phy {
>>
>>       /* for notification of usb_phy_events */
>>       struct atomic_notifier_head     notifier;
>> +     struct notifier_block   otgws_nb;
>
> drop this notifier block.
>
>> +
>> +     /* wakeup source */
>> +     struct wakeup_source    wsource;
>
> this is the only thing you need.
>
>> +
>> +     spinlock_t              otgws_slock;
>
> drop this lock.
>
> --
> balbi

Re: [RFC v4] usb: phy: Hold wakeupsource when USB is enumerated in peripheral mode

[Felipe Balbi]

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

Hi,

On Fri, Oct 10, 2014 at 11:37:31AM +0530, Kiran Raparthy wrote:
> Hi Felipe,
> Thank you very much for taking time in reviewing the patch.
> I will try to improve the patch as per your suggestions.
> however,i have few queries which i wanted to understand from you.

sure thing.

> On 7 October 2014 19:55, Felipe Balbi <balbi@ti.com> wrote:
> >> +static int otg_wakeupsource_init(void)
> >> +{
> >> +     int ret_usb2;
> >> +     int ret_usb3;
> >> +     char wsource_name_usb2[40];
> >> +     char wsource_name_usb3[40];
> >> +     static struct usb_phy *otgws_xceiv_usb2;
> >> +     static struct usb_phy *otgws_xceiv_usb3;
> >> +
> >> +     otgws_xceiv_usb2 = usb_get_phy(USB_PHY_TYPE_USB2);
> >> +     otgws_xceiv_usb3 = usb_get_phy(USB_PHY_TYPE_USB3);
> >> +
> >> +     if (IS_ERR(otgws_xceiv_usb2) && IS_ERR(otgws_xceiv_usb3)) {
> >> +             pr_err("%s: No OTG transceiver found\n", __func__);
> >> +             return PTR_ERR(otgws_xceiv_usb2);
> >> +     }
> >> +
> >> +     spin_lock_init(&otgws_xceiv_usb2->otgws_slock);
> >> +     spin_lock_init(&otgws_xceiv_usb3->otgws_slock);
> >> +
> >> +     snprintf(wsource_name_usb2, sizeof(wsource_name_usb2), "vbus-%s",
> >> +             dev_name(otgws_xceiv_usb2->dev));
> >> +     wakeup_source_init(&otgws_xceiv_usb2->wsource, wsource_name_usb2);
> >> +
> >> +     snprintf(wsource_name_usb3, sizeof(wsource_name_usb3), "vbus-%s",
> >> +             dev_name(otgws_xceiv_usb3->dev));
> >> +     wakeup_source_init(&otgws_xceiv_usb3->wsource, wsource_name_usb3);
> >> +
> >> +     otgws_xceiv_usb2->otgws_nb.notifier_call = otgws_otg_usb2_notifications;
> >> +     ret_usb2 = usb_register_notifier(otgws_xceiv_usb2,
> >> +                                     &otgws_xceiv_usb2->otgws_nb);
> >> +
> >> +     otgws_xceiv_usb3->otgws_nb.notifier_call = otgws_otg_usb3_notifications;
> >> +     ret_usb3 = usb_register_notifier(otgws_xceiv_usb3,
> >> +                                     &otgws_xceiv_usb3->otgws_nb);
> >> +
> >> +     if (ret_usb2 && ret_usb3) {
> >> +             pr_err("%s: usb_register_notifier on transceiver failed\n",
> >> +                      __func__);
> >> +             wakeup_source_trash(&otgws_xceiv_usb2->wsource);
> >> +             wakeup_source_trash(&otgws_xceiv_usb3->wsource);
> >> +             otgws_xceiv_usb2 = NULL;
> >> +             otgws_xceiv_usb3 = NULL;
> >> +             return ret_usb2 | ret_usb3;
> >> +     }
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +late_initcall(otg_wakeupsource_init);
> >
> > you guys are really not getting what I mean. I asked for this to be
> > built into the core itself. This means that you shouldn't need to use
> > notifications nor should you need to call usb_get_phy(). You're part of
> > the PHY framework.
> >
> > All this late_initcall() nonsense should go.
> >
> > This code won't even work if we have more than one phy of the same type
> > (AM437x SoC, for example, has up to 4 instances of dwc3, so that's 4
> > USB2 PHYs), because you can't grab the PHY you want.
> 
> Apologies,I am new to usb sub system,so i missed this point before i
> posted my patch,Thanks for the information.

np.

> > What you need is to:
> >
> > 1) make PHY notifiers generic (move all of that phy-core.c)
> From the above points,you mentioned that "if we built it into core,we
> shouldn't need to use notifications"
> and your first point here says that make phy notifiers generic in phy-core.c
> can you help me understanding it better so that there wont be any
> understanding gap.

yeah, notifiers should go but if you really must use them, then at least
make all of that generic ;-)

> > 2) introduce usb_phy_set_event(phy, event) (which just sets the even to a
> >         phy->event member for now)
> > 3) make all PHY drivers use usb_phy_set_event()
> > 4) add the following to usb_phy_set_event()
> >
> >         switch (event) {
> >         case USB_EVENT_ENUMERATED:
> >                 pm_stay_awake(&otgws_xceiv->wsource);
> >                 break;
> >
> >         case USB_EVENT_NONE:
> >         case USB_EVENT_VBUS:
> >         case USB_EVENT_ID:
> >         case USB_EVENT_CHARGER:
> >                 pm_wakeup_event(&otgws_xceiv->wsource,
> >                                 msecs_to_jiffies(TEMPORARY_HOLD_TIME));
> >                 break;
> >
> >         default:
> >                 break;
> >         }
> >
> Once the phy drivers receives per-PHY event notification(if we use
> notifier,else "for any event") we can call usb_phy_set_event from phy
> driver to hold the wakeup source.
> Please correct me if my understanding is incorrect.

yeah. In fact, you can call usb_phy_set_event() directly from PHY's IRQ
handler.

> I have gone through some phy drivers in drivers/phy,since the each
> driver implementation is different from others, i didn't get the best
> place in  PHY driver
> where we can trigger(use phy-core functionality) per-PHY notifier
> registration. any pointers here?

registration ? probe(), they all have probe() functions. Now to figure
out where to call usb_phy_set_event(). That's something completely
different, and that's where the core of this change is :-)

For PHYs which have IRQ lines, easy: just call usb_phy_set_event() from
IRQ handler. For those who don't, then it's a little more difficult and
will require your investigation.

-- 
balbi

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

Re: [RFC v4] usb: phy: Hold wakeupsource when USB is enumerated in peripheral mode

[Kiran Raparthy]

On 10 October 2014 20:50, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Fri, Oct 10, 2014 at 11:37:31AM +0530, Kiran Raparthy wrote:
>> Hi Felipe,
>> Thank you very much for taking time in reviewing the patch.
>> I will try to improve the patch as per your suggestions.
>> however,i have few queries which i wanted to understand from you.
>
> sure thing.
>
>> On 7 October 2014 19:55, Felipe Balbi <balbi@ti.com> wrote:
>> >> +static int otg_wakeupsource_init(void)
>> >> +{
>> >> +     int ret_usb2;
>> >> +     int ret_usb3;
>> >> +     char wsource_name_usb2[40];
>> >> +     char wsource_name_usb3[40];
>> >> +     static struct usb_phy *otgws_xceiv_usb2;
>> >> +     static struct usb_phy *otgws_xceiv_usb3;
>> >> +
>> >> +     otgws_xceiv_usb2 = usb_get_phy(USB_PHY_TYPE_USB2);
>> >> +     otgws_xceiv_usb3 = usb_get_phy(USB_PHY_TYPE_USB3);
>> >> +
>> >> +     if (IS_ERR(otgws_xceiv_usb2) && IS_ERR(otgws_xceiv_usb3)) {
>> >> +             pr_err("%s: No OTG transceiver found\n", __func__);
>> >> +             return PTR_ERR(otgws_xceiv_usb2);
>> >> +     }
>> >> +
>> >> +     spin_lock_init(&otgws_xceiv_usb2->otgws_slock);
>> >> +     spin_lock_init(&otgws_xceiv_usb3->otgws_slock);
>> >> +
>> >> +     snprintf(wsource_name_usb2, sizeof(wsource_name_usb2), "vbus-%s",
>> >> +             dev_name(otgws_xceiv_usb2->dev));
>> >> +     wakeup_source_init(&otgws_xceiv_usb2->wsource, wsource_name_usb2);
>> >> +
>> >> +     snprintf(wsource_name_usb3, sizeof(wsource_name_usb3), "vbus-%s",
>> >> +             dev_name(otgws_xceiv_usb3->dev));
>> >> +     wakeup_source_init(&otgws_xceiv_usb3->wsource, wsource_name_usb3);
>> >> +
>> >> +     otgws_xceiv_usb2->otgws_nb.notifier_call = otgws_otg_usb2_notifications;
>> >> +     ret_usb2 = usb_register_notifier(otgws_xceiv_usb2,
>> >> +                                     &otgws_xceiv_usb2->otgws_nb);
>> >> +
>> >> +     otgws_xceiv_usb3->otgws_nb.notifier_call = otgws_otg_usb3_notifications;
>> >> +     ret_usb3 = usb_register_notifier(otgws_xceiv_usb3,
>> >> +                                     &otgws_xceiv_usb3->otgws_nb);
>> >> +
>> >> +     if (ret_usb2 && ret_usb3) {
>> >> +             pr_err("%s: usb_register_notifier on transceiver failed\n",
>> >> +                      __func__);
>> >> +             wakeup_source_trash(&otgws_xceiv_usb2->wsource);
>> >> +             wakeup_source_trash(&otgws_xceiv_usb3->wsource);
>> >> +             otgws_xceiv_usb2 = NULL;
>> >> +             otgws_xceiv_usb3 = NULL;
>> >> +             return ret_usb2 | ret_usb3;
>> >> +     }
>> >> +
>> >> +     return 0;
>> >> +}
>> >> +
>> >> +late_initcall(otg_wakeupsource_init);
>> >
>> > you guys are really not getting what I mean. I asked for this to be
>> > built into the core itself. This means that you shouldn't need to use
>> > notifications nor should you need to call usb_get_phy(). You're part of
>> > the PHY framework.
>> >
>> > All this late_initcall() nonsense should go.
>> >
>> > This code won't even work if we have more than one phy of the same type
>> > (AM437x SoC, for example, has up to 4 instances of dwc3, so that's 4
>> > USB2 PHYs), because you can't grab the PHY you want.
>>
>> Apologies,I am new to usb sub system,so i missed this point before i
>> posted my patch,Thanks for the information.
>
> np.
>
>> > What you need is to:
>> >
>> > 1) make PHY notifiers generic (move all of that phy-core.c)
>> From the above points,you mentioned that "if we built it into core,we
>> shouldn't need to use notifications"
>> and your first point here says that make phy notifiers generic in phy-core.c
>> can you help me understanding it better so that there wont be any
>> understanding gap.
>
> yeah, notifiers should go but if you really must use them, then at least
> make all of that generic ;-)
>
>> > 2) introduce usb_phy_set_event(phy, event) (which just sets the even to a
>> >         phy->event member for now)
>> > 3) make all PHY drivers use usb_phy_set_event()
>> > 4) add the following to usb_phy_set_event()
>> >
>> >         switch (event) {
>> >         case USB_EVENT_ENUMERATED:
>> >                 pm_stay_awake(&otgws_xceiv->wsource);
>> >                 break;
>> >
>> >         case USB_EVENT_NONE:
>> >         case USB_EVENT_VBUS:
>> >         case USB_EVENT_ID:
>> >         case USB_EVENT_CHARGER:
>> >                 pm_wakeup_event(&otgws_xceiv->wsource,
>> >                                 msecs_to_jiffies(TEMPORARY_HOLD_TIME));
>> >                 break;
>> >
>> >         default:
>> >                 break;
>> >         }
>> >
>> Once the phy drivers receives per-PHY event notification(if we use
>> notifier,else "for any event") we can call usb_phy_set_event from phy
>> driver to hold the wakeup source.
>> Please correct me if my understanding is incorrect.
>
> yeah. In fact, you can call usb_phy_set_event() directly from PHY's IRQ
> handler.
>
>> I have gone through some phy drivers in drivers/phy,since the each
>> driver implementation is different from others, i didn't get the best
>> place in  PHY driver
>> where we can trigger(use phy-core functionality) per-PHY notifier
>> registration. any pointers here?
>
> registration ? probe(), they all have probe() functions. Now to figure
> out where to call usb_phy_set_event(). That's something completely
> different, and that's where the core of this change is :-)
>
> For PHYs which have IRQ lines, easy: just call usb_phy_set_event() from
> IRQ handler. For those who don't, then it's a little more difficult and
> will require your investigation.
Thanks for your inputs.
Regards,
Kiran
>
> --
> balbi

Re: [RFC v4] usb: phy: Hold wakeupsource when USB is enumerated in peripheral mode

[Kiran Raparthy]

HI Felipe,

On 27 October 2014 15:06, Kiran Raparthy <kiran.kumar@linaro.org> wrote:
> Hi Felipe,
>
> On 10 October 2014 20:50, Felipe Balbi <balbi@ti.com> wrote:
>> Hi,
>>
>> On Fri, Oct 10, 2014 at 11:37:31AM +0530, Kiran Raparthy wrote:
>>> Hi Felipe,
>>> Thank you very much for taking time in reviewing the patch.
>>> I will try to improve the patch as per your suggestions.
>>> however,i have few queries which i wanted to understand from you.
>>
>> sure thing.
>>
>>> On 7 October 2014 19:55, Felipe Balbi <balbi@ti.com> wrote:
>>> >> +static int otg_wakeupsource_init(void)
>>> >> +{
>>> >> +     int ret_usb2;
>>> >> +     int ret_usb3;
>>> >> +     char wsource_name_usb2[40];
>>> >> +     char wsource_name_usb3[40];
>>> >> +     static struct usb_phy *otgws_xceiv_usb2;
>>> >> +     static struct usb_phy *otgws_xceiv_usb3;
>>> >> +
>>> >> +     otgws_xceiv_usb2 = usb_get_phy(USB_PHY_TYPE_USB2);
>>> >> +     otgws_xceiv_usb3 = usb_get_phy(USB_PHY_TYPE_USB3);
>>> >> +
>>> >> +     if (IS_ERR(otgws_xceiv_usb2) && IS_ERR(otgws_xceiv_usb3)) {
>>> >> +             pr_err("%s: No OTG transceiver found\n", __func__);
>>> >> +             return PTR_ERR(otgws_xceiv_usb2);
>>> >> +     }
>>> >> +
>>> >> +     spin_lock_init(&otgws_xceiv_usb2->otgws_slock);
>>> >> +     spin_lock_init(&otgws_xceiv_usb3->otgws_slock);
>>> >> +
>>> >> +     snprintf(wsource_name_usb2, sizeof(wsource_name_usb2),
>>> >> "vbus-%s",
>>> >> +             dev_name(otgws_xceiv_usb2->dev));
>>> >> +     wakeup_source_init(&otgws_xceiv_usb2->wsource,
>>> >> wsource_name_usb2);
>>> >> +
>>> >> +     snprintf(wsource_name_usb3, sizeof(wsource_name_usb3),
>>> >> "vbus-%s",
>>> >> +             dev_name(otgws_xceiv_usb3->dev));
>>> >> +     wakeup_source_init(&otgws_xceiv_usb3->wsource,
>>> >> wsource_name_usb3);
>>> >> +
>>> >> +     otgws_xceiv_usb2->otgws_nb.notifier_call =
>>> >> otgws_otg_usb2_notifications;
>>> >> +     ret_usb2 = usb_register_notifier(otgws_xceiv_usb2,
>>> >> +                                     &otgws_xceiv_usb2->otgws_nb);
>>> >> +
>>> >> +     otgws_xceiv_usb3->otgws_nb.notifier_call =
>>> >> otgws_otg_usb3_notifications;
>>> >> +     ret_usb3 = usb_register_notifier(otgws_xceiv_usb3,
>>> >> +                                     &otgws_xceiv_usb3->otgws_nb);
>>> >> +
>>> >> +     if (ret_usb2 && ret_usb3) {
>>> >> +             pr_err("%s: usb_register_notifier on transceiver
>>> >> failed\n",
>>> >> +                      __func__);
>>> >> +             wakeup_source_trash(&otgws_xceiv_usb2->wsource);
>>> >> +             wakeup_source_trash(&otgws_xceiv_usb3->wsource);
>>> >> +             otgws_xceiv_usb2 = NULL;
>>> >> +             otgws_xceiv_usb3 = NULL;
>>> >> +             return ret_usb2 | ret_usb3;
>>> >> +     }
>>> >> +
>>> >> +     return 0;
>>> >> +}
>>> >> +
>>> >> +late_initcall(otg_wakeupsource_init);
>>> >
>>> > you guys are really not getting what I mean. I asked for this to be
>>> > built into the core itself. This means that you shouldn't need to use
>>> > notifications nor should you need to call usb_get_phy(). You're part of
>>> > the PHY framework.
>>> >
>>> > All this late_initcall() nonsense should go.
>>> >
>>> > This code won't even work if we have more than one phy of the same type
>>> > (AM437x SoC, for example, has up to 4 instances of dwc3, so that's 4
>>> > USB2 PHYs), because you can't grab the PHY you want.
>>>
>>> Apologies,I am new to usb sub system,so i missed this point before i
>>> posted my patch,Thanks for the information.
>>
>> np.
>>
>>> > What you need is to:
>>> >
>>> > 1) make PHY notifiers generic (move all of that phy-core.c)
>>> From the above points,you mentioned that "if we built it into core,we
>>> shouldn't need to use notifications"
>>> and your first point here says that make phy notifiers generic in
>>> phy-core.c
>>> can you help me understanding it better so that there wont be any
>>> understanding gap.
>>
>> yeah, notifiers should go but if you really must use them, then at least
>> make all of that generic ;-)
>>
>>> > 2) introduce usb_phy_set_event(phy, event) (which just sets the even to
>>> > a
>>> >         phy->event member for now)
>>> > 3) make all PHY drivers use usb_phy_set_event()
>>> > 4) add the following to usb_phy_set_event()
>>> >
>>> >         switch (event) {
>>> >         case USB_EVENT_ENUMERATED:
>>> >                 pm_stay_awake(&otgws_xceiv->wsource);
>>> >                 break;
>>> >
>>> >         case USB_EVENT_NONE:
>>> >         case USB_EVENT_VBUS:
>>> >         case USB_EVENT_ID:
>>> >         case USB_EVENT_CHARGER:
>>> >                 pm_wakeup_event(&otgws_xceiv->wsource,
>>> >                                 msecs_to_jiffies(TEMPORARY_HOLD_TIME));
>>> >                 break;
>>> >
>>> >         default:
>>> >                 break;
>>> >         }
>>> >
>>> Once the phy drivers receives per-PHY event notification(if we use
>>> notifier,else "for any event") we can call usb_phy_set_event from phy
>>> driver to hold the wakeup source.
>>> Please correct me if my understanding is incorrect.
>>
>> yeah. In fact, you can call usb_phy_set_event() directly from PHY's IRQ
>> handler.
>>
>>> I have gone through some phy drivers in drivers/phy,since the each
>>> driver implementation is different from others, i didn't get the best
>>> place in  PHY driver
>>> where we can trigger(use phy-core functionality) per-PHY notifier
>>> registration. any pointers here?
>>
>> registration ? probe(), they all have probe() functions. Now to figure
>> out where to call usb_phy_set_event(). That's something completely
>> different, and that's where the core of this change is :-)
>>
>> For PHYs which have IRQ lines, easy: just call usb_phy_set_event() from
>> IRQ handler. For those who don't, then it's a little more difficult and
>> will require your investigation.
>
just a gentle reminder, can you have a look at below points and share
your thoughts?

> I am following below approach,please suggest/correct me, if you feel
> something is wrong.
>
> 1. Adding usb_phy_set_event function in drivers/usb/phy/phy.c(you asked me
> to add this in phy-core.c but i felt phy.c is right place to add this
> function).
> 2. Also add usb_phy_wsource_init and usb_phy_wsource_trash in phy.c so that
> PHY drivers can use these interfaces to initialize and trash per-PHY
> wakeupsource.
> 3. call usb_phy_wsource_init from probe and usb_phy_wsource_trash from probe
> and xxx_remove functions respectively in PHY drivers.
> 4. call usb_phy_set_event from PHY drivers where complete USB enumeration(as
> peripheral) event is handled(not simply on VBUS detection).
> 5. As of now,i am not using any notification mechanism.
>
> Below are some issues for which i need your suggestions:
> 1. In previous patches(till V3), i have used "enabled" field which is a
> module param(/sys/module/otg_wakeupsource/parameters/enabled) to disallow
> "holding wakeupsource".
>
>     your comment for the above field was "This shouldn't be kernel
> parameter" and you asked me to drop it,Just wanted to understand whether you
> want me to drop it completely
>     or implement it per-PHY(if we need to implement it per-PHY,we may have
> to add module param entry in each PHY driver which leads to N number of
> sysfs entries).
>
>     If you still prefers to have this functionality, can we use single
> "enabled" field for all PHY's?(to avoid N number of sysfs entries,just
> wanted to check if this is okay?)
>     If you want me to remove this field completely,then i can make change
> accordingly to my patch.Please clarify.
>
> 2. I have gone through all the PHY drivers,but i could able to change only 6
> drivers to use wakeup source mechanism(call usb_phy_set_event after USB
> enumerated in peripheral
>     mode).Is it okay if i submit the patch modifying only 6 PHY drivers as
> initial patch?
>
>     I have classified as below based on my observations(please let me know
> if you have any suggestions).
>
>     I have modified below phy drivers to use wakeupsource mechanism:
>     drivers/phy/phy-omap-control.c
>     drivers/usb/phy/phy-ab8500-usb.c
>     phy-gpio-vbus-usb.c
>     drivers/usb/phy/phy-tahvo.c
>     drivers/usb/phy/phy-mv-usb.c
>     drivers/usb/phy/phy-gpio-vbus-usb.c
>
>     For below phy drivers,no PHY events(Enumeration in peripheral mode) are
> handled in driver
>     drivers/phy/phy-bcm-kona-usb2.c
>     drivers/phy/phy-exynos4210-usb2.c
>     drivers/phy/phy-exynos4x12-usb2.c
>     drivers/phy/phy-exynos5250-sata.c
>     drivers/phy/phy-exynos5-usbdrd.c
>     drivers/phy/phy-exynos-dp-video.c
>     drivers/phy/phy-exynos-mipi-video.c
>     drivers/phy/phy-mvebu-sata.c
>     drivers/phy/phy-samsung-usb2.c
>     drivers/phy/phy-sun4i-usb.c
>     drivers/phy/phy-ti-pipe3.c
>     drivers/phy/phy-xgene.c
>     drivers/phy/phy-omap-usb2.c
>     drivers/phy/phy-twl4030-usb.c
>     drivers/usb/phy/phy-am335x.c
>     drivers/usb/phy/phy-am335x-control.c
>     drivers/usb/phy/phy-generic.c
>     drivers/usb/phy/phy-isp1301.c
>     drivers/usb/phy/phy-rcar-gen2-usb.c
>     drivers/usb/phy/phy-rcar-usb.c
>     drivers/usb/phy/phy-samsung-usb2.c
>     drivers/usb/phy/phy-samsung-usb3.c
>     drivers/usb/phy/phy-samsung-usb.c
>     drivers/usb/phy/phy-tegra-usb.c
>     drivers/usb/phy/phy-ulpi-viewport.c
>     drivers/usb/phy/phy-keystone.c
>     drivers/usb/phy/phy-mxs-usb.c
>     drivers/usb/phy-omap-otg.c
>     drivers/usb/phy/phy-ulpi.c
>
>     For below PHY driver,disconnect event not handled,so we can't hold
> wakeupsource.
>     drivers/usb/phy/phy-fsl-usb.c
>
>     For below PHY driver,only VBUS events are handled(Enumeration event not
> handled).
>     drivers/usb/phy/phy-twl6030-usb.c
>
>     For below PHY drivers,i am not clear where to call usb_phy_set_event
>     drivers/usb/phy/phy-isp1301-omap.c
>     drivers/usb/phy/phy-msm-usb.c
>
>
Regards,
Kiran
> Regards,
> Kiran
>>
>> --
>> balbi

Re: [RFC v4] usb: phy: Hold wakeupsource when USB is enumerated in peripheral mode

[Felipe Balbi]

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

Hi,

On Fri, Oct 31, 2014 at 09:27:43AM +0530, Kiran Raparthy wrote:
> >>> Thank you very much for taking time in reviewing the patch.
> >>> I will try to improve the patch as per your suggestions.
> >>> however,i have few queries which i wanted to understand from you.
> >>
> >> sure thing.
> >>
> >>> On 7 October 2014 19:55, Felipe Balbi <balbi@ti.com> wrote:
> >>> >> +static int otg_wakeupsource_init(void)
> >>> >> +{
> >>> >> +     int ret_usb2;
> >>> >> +     int ret_usb3;
> >>> >> +     char wsource_name_usb2[40];
> >>> >> +     char wsource_name_usb3[40];
> >>> >> +     static struct usb_phy *otgws_xceiv_usb2;
> >>> >> +     static struct usb_phy *otgws_xceiv_usb3;
> >>> >> +
> >>> >> +     otgws_xceiv_usb2 = usb_get_phy(USB_PHY_TYPE_USB2);
> >>> >> +     otgws_xceiv_usb3 = usb_get_phy(USB_PHY_TYPE_USB3);
> >>> >> +
> >>> >> +     if (IS_ERR(otgws_xceiv_usb2) && IS_ERR(otgws_xceiv_usb3)) {
> >>> >> +             pr_err("%s: No OTG transceiver found\n", __func__);
> >>> >> +             return PTR_ERR(otgws_xceiv_usb2);
> >>> >> +     }
> >>> >> +
> >>> >> +     spin_lock_init(&otgws_xceiv_usb2->otgws_slock);
> >>> >> +     spin_lock_init(&otgws_xceiv_usb3->otgws_slock);
> >>> >> +
> >>> >> +     snprintf(wsource_name_usb2, sizeof(wsource_name_usb2),
> >>> >> "vbus-%s",
> >>> >> +             dev_name(otgws_xceiv_usb2->dev));
> >>> >> +     wakeup_source_init(&otgws_xceiv_usb2->wsource,
> >>> >> wsource_name_usb2);
> >>> >> +
> >>> >> +     snprintf(wsource_name_usb3, sizeof(wsource_name_usb3),
> >>> >> "vbus-%s",
> >>> >> +             dev_name(otgws_xceiv_usb3->dev));
> >>> >> +     wakeup_source_init(&otgws_xceiv_usb3->wsource,
> >>> >> wsource_name_usb3);
> >>> >> +
> >>> >> +     otgws_xceiv_usb2->otgws_nb.notifier_call =
> >>> >> otgws_otg_usb2_notifications;
> >>> >> +     ret_usb2 = usb_register_notifier(otgws_xceiv_usb2,
> >>> >> +                                     &otgws_xceiv_usb2->otgws_nb);
> >>> >> +
> >>> >> +     otgws_xceiv_usb3->otgws_nb.notifier_call =
> >>> >> otgws_otg_usb3_notifications;
> >>> >> +     ret_usb3 = usb_register_notifier(otgws_xceiv_usb3,
> >>> >> +                                     &otgws_xceiv_usb3->otgws_nb);
> >>> >> +
> >>> >> +     if (ret_usb2 && ret_usb3) {
> >>> >> +             pr_err("%s: usb_register_notifier on transceiver
> >>> >> failed\n",
> >>> >> +                      __func__);
> >>> >> +             wakeup_source_trash(&otgws_xceiv_usb2->wsource);
> >>> >> +             wakeup_source_trash(&otgws_xceiv_usb3->wsource);
> >>> >> +             otgws_xceiv_usb2 = NULL;
> >>> >> +             otgws_xceiv_usb3 = NULL;
> >>> >> +             return ret_usb2 | ret_usb3;
> >>> >> +     }
> >>> >> +
> >>> >> +     return 0;
> >>> >> +}
> >>> >> +
> >>> >> +late_initcall(otg_wakeupsource_init);
> >>> >
> >>> > you guys are really not getting what I mean. I asked for this to be
> >>> > built into the core itself. This means that you shouldn't need to use
> >>> > notifications nor should you need to call usb_get_phy(). You're part of
> >>> > the PHY framework.
> >>> >
> >>> > All this late_initcall() nonsense should go.
> >>> >
> >>> > This code won't even work if we have more than one phy of the same type
> >>> > (AM437x SoC, for example, has up to 4 instances of dwc3, so that's 4
> >>> > USB2 PHYs), because you can't grab the PHY you want.
> >>>
> >>> Apologies,I am new to usb sub system,so i missed this point before i
> >>> posted my patch,Thanks for the information.
> >>
> >> np.
> >>
> >>> > What you need is to:
> >>> >
> >>> > 1) make PHY notifiers generic (move all of that phy-core.c)
> >>> From the above points,you mentioned that "if we built it into core,we
> >>> shouldn't need to use notifications"
> >>> and your first point here says that make phy notifiers generic in
> >>> phy-core.c
> >>> can you help me understanding it better so that there wont be any
> >>> understanding gap.
> >>
> >> yeah, notifiers should go but if you really must use them, then at least
> >> make all of that generic ;-)
> >>
> >>> > 2) introduce usb_phy_set_event(phy, event) (which just sets the even to
> >>> > a
> >>> >         phy->event member for now)
> >>> > 3) make all PHY drivers use usb_phy_set_event()
> >>> > 4) add the following to usb_phy_set_event()
> >>> >
> >>> >         switch (event) {
> >>> >         case USB_EVENT_ENUMERATED:
> >>> >                 pm_stay_awake(&otgws_xceiv->wsource);
> >>> >                 break;
> >>> >
> >>> >         case USB_EVENT_NONE:
> >>> >         case USB_EVENT_VBUS:
> >>> >         case USB_EVENT_ID:
> >>> >         case USB_EVENT_CHARGER:
> >>> >                 pm_wakeup_event(&otgws_xceiv->wsource,
> >>> >                                 msecs_to_jiffies(TEMPORARY_HOLD_TIME));
> >>> >                 break;
> >>> >
> >>> >         default:
> >>> >                 break;
> >>> >         }
> >>> >
> >>> Once the phy drivers receives per-PHY event notification(if we use
> >>> notifier,else "for any event") we can call usb_phy_set_event from phy
> >>> driver to hold the wakeup source.
> >>> Please correct me if my understanding is incorrect.
> >>
> >> yeah. In fact, you can call usb_phy_set_event() directly from PHY's IRQ
> >> handler.
> >>
> >>> I have gone through some phy drivers in drivers/phy,since the each
> >>> driver implementation is different from others, i didn't get the best
> >>> place in  PHY driver
> >>> where we can trigger(use phy-core functionality) per-PHY notifier
> >>> registration. any pointers here?
> >>
> >> registration ? probe(), they all have probe() functions. Now to figure
> >> out where to call usb_phy_set_event(). That's something completely
> >> different, and that's where the core of this change is :-)
> >>
> >> For PHYs which have IRQ lines, easy: just call usb_phy_set_event() from
> >> IRQ handler. For those who don't, then it's a little more difficult and
> >> will require your investigation.
> >
> just a gentle reminder, can you have a look at below points and share
> your thoughts?

Send the patch, I have reviewed this multiple times and all those
comments are archived on multiple mailing list archives. Just read them
again if you need.

-- 
balbi

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