Discussion:
[RFC v4] usb: phy: Hold wakeupsource when USB is enumerated in peripheral mode
Kiran Kumar Raparthy
2014-10-07 09:15:44 UTC
Permalink
=46rom: Todd Poynor <***@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 fr=
om 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 all=
ow
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 <***@ti.com>
Cc: Greg Kroah-Hartman <***@linuxfoundation.org>
Cc: linux-***@vger.kernel.org
Cc: linux-***@vger.kernel.org
Cc: Android Kernel Team <kernel-***@android.com>
Cc: John Stultz <***@linaro.org>
Cc: Sumit Semwal <***@linaro.org>
Cc: Arve Hj=F8nnev=E5g <***@android.com>
Cc: Benoit Goby <***@android.com>
Signed-off-by: Todd Poynor <***@google.com>
[kiran: Added context to commit message, squished build fixes
from Benoit Goby and Arve Hj=F8nnev=E5g, 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 <***@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=F8nnev=E5g
* 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
=20
+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) +=3D phy.o
obj-$(CONFIG_OF) +=3D of.o
+obj-$(CONFIG_USB_OTG_WAKEUPSOURCE) +=3D otg-wakeupsource.o
=20
# transceiver drivers, keep the list sorted
=20
diff --git a/drivers/usb/phy/otg-wakeupsource.c b/drivers/usb/phy/otg-w=
akeupsource.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, an=
d
+ * 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 l=
ong 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 =3D 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 =3D 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 =3D usb_get_phy(USB_PHY_TYPE_USB2);
+ otgws_xceiv_usb3 =3D 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 =3D otgws_otg_usb2_notificat=
ions;
+ ret_usb2 =3D usb_register_notifier(otgws_xceiv_usb2,
+ &otgws_xceiv_usb2->otgws_nb);
+
+ otgws_xceiv_usb3->otgws_nb.notifier_call =3D otgws_otg_usb3_notificat=
ions;
+ ret_usb3 =3D 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 =3D NULL;
+ otgws_xceiv_usb3 =3D 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>
=20
+#define TEMPORARY_HOLD_TIME 2000
+
enum usb_phy_interface {
USBPHY_INTERFACE_MODE_UNKNOWN,
USBPHY_INTERFACE_MODE_UTMI,
@@ -88,6 +90,12 @@ struct usb_phy {
=20
/* 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;
=20
/* to pass extra port status to the root hub */
u16 port_status;
--=20
1.8.2.1
Felipe Balbi
2014-10-07 14:25:54 UTC
Permalink
Hi,
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) {
Looks like VBUS should be temporary too.
+ __pm_stay_awake(&otgws_xceiv->wsource);
+ break;
+
+ __pm_wakeup_event(&otgws_xceiv->wsource,
+ msecs_to_jiffies(TEMPORARY_HOLD_TIME));
+ break;
+
+ 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
Kiran Raparthy
2014-10-10 06:07:31 UTC
Permalink
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.
Post by Felipe Balbi
Hi,
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) {
Looks like VBUS should be temporary too.
+ __pm_stay_awake(&otgws_xceiv->wsource);
+ break;
+
+ __pm_wakeup_event(&otgws_xceiv->wsource,
+ msecs_to_jiffies(TEMPORARY_HOLD_TIME));
+ break;
+
+ 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.
Post by Felipe Balbi
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.
Post by Felipe Balbi
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) {
pm_stay_awake(&otgws_xceiv->wsource);
break;
pm_wakeup_event(&otgws_xceiv->wsource,
msecs_to_jiffies(TEMPORARY_HOLD_TIME));
break;
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
Post by Felipe Balbi
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Felipe Balbi
2014-10-10 15:20:44 UTC
Permalink
Hi,
Post by 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.
sure thing.
Post by Kiran Raparthy
Post by Felipe Balbi
Post by Kiran Kumar Raparthy
+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.
Post by Kiran Raparthy
Post by Felipe Balbi
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 ;-)
Post by Kiran Raparthy
Post by Felipe Balbi
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) {
pm_stay_awake(&otgws_xceiv->wsource);
break;
pm_wakeup_event(&otgws_xceiv->wsource,
msecs_to_jiffies(TEMPORARY_HOLD_TIME));
break;
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.
Post by Kiran Raparthy
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
Kiran Raparthy
2014-10-13 03:46:02 UTC
Permalink
Post by Felipe Balbi
Hi,
Post by 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.
sure thing.
Post by Kiran Raparthy
Post by Felipe Balbi
Post by Kiran Kumar Raparthy
+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.
Post by Kiran Raparthy
Post by Felipe Balbi
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 ;-)
Post by Kiran Raparthy
Post by Felipe Balbi
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) {
pm_stay_awake(&otgws_xceiv->wsource);
break;
pm_wakeup_event(&otgws_xceiv->wsource,
msecs_to_jiffies(TEMPORARY_HOLD_TIME));
break;
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.
Post by Kiran Raparthy
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
Post by Felipe Balbi
--
balbi
Loading...