[RFC 0/5] USB core changes for supporting OTG on MSM SoC

[RFC 0/5] USB core changes for supporting OTG on MSM SoC

[Pavankumar Kondeti]

This patch series adds OTG 2.0 enhancements and misc changes
required for supporting SRP & HNP in MSM OTG driver.  As these
patches modify the USB core code, I would like to receive
feedback before posting driver patches. The driver patches are
available at
https://www.codeaurora.org/gitweb/quic/la/?p=kernel/msm.git;a=commitdiff;h=05535a995e9eb34c3c41d874d09955a443008cd5

Pavankumar Kondeti (5):
  USB: core: Add input prompt and help text for USB_OTG config
  USB: core: OTG Supplement Revision 2.0 updates
  USB: gadget: OTG supplement revision 2.0 updates
  USB: EHCI: Notify HCD about HNP enabled port suspend
  USB: Eliminate delays involved in root hub initialization during HNP

 .../testing/sysfs-devices-platform-_UDC_-gadget    |   14 +++
 drivers/usb/core/Kconfig                           |   12 +++-
 drivers/usb/core/driver.c                          |   50 ++++++++++++
 drivers/usb/core/hcd.c                             |    7 ++
 drivers/usb/core/hub.c                             |   84 +++++++++++++++++---
 drivers/usb/core/usb.h                             |    4 +
 drivers/usb/gadget/composite.c                     |   25 ++++++-
 drivers/usb/gadget/zero.c                          |    3 +
 drivers/usb/host/ehci-hub.c                        |   11 +++
 drivers/usb/host/ehci.h                            |    2 +
 include/linux/usb.h                                |    2 +
 include/linux/usb/ch9.h                            |   11 ++-
 include/linux/usb/gadget.h                         |   21 +++++
 13 files changed, 231 insertions(+), 15 deletions(-)

--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[RFC 1/5] USB: core: Add input prompt and help text for USB_OTG config

[Pavankumar Kondeti]

bd6882 commit removes the duplicate USB_OTG config from
gadget/Kconfig.  But does not copy the input prompt and
help text to the original config defined in core/Kconfig.
Add them now.

Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
---
 drivers/usb/core/Kconfig |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
index 9eed5b5..bcc2477 100644
--- a/drivers/usb/core/Kconfig
+++ b/drivers/usb/core/Kconfig
@@ -107,11 +107,19 @@ config USB_SUSPEND
 	  If you are unsure about this, say N here.
 
 config USB_OTG
-	bool
+	bool "OTG support"
 	depends on USB && EXPERIMENTAL
 	depends on USB_SUSPEND
 	default n
-
+	help
+	  The most notable feature of USB OTG is support for a
+	  "Dual-Role" device, which can act as either a device
+	  or a host. The initial role is decided by the type of
+	  plug inserted and can be changed later when two dual
+	  role devices talk to each other.
+
+	  Select this only if your board has Mini-AB/Micro-AB
+	  connector.
 
 config USB_OTG_WHITELIST
 	bool "Rely on OTG Targeted Peripherals List"
-- 
1.7.1

--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [RFC 1/5] USB: core: Add input prompt and help text for USB_OTG config

[Sergei Shtylyov]

Hello.

On 15-12-2010 14:14, Pavankumar Kondeti wrote:

> bd6882 commit removes the duplicate USB_OTG config from

    Linus has asked to also specify the commit summary in parens.

> gadget/Kconfig.  But does not copy the input prompt and
> help text to the original config defined in core/Kconfig.
> Add them now.

> Signed-off-by: Pavankumar Kondeti<pkondeti@codeaurora.org>

WBR, Sergei

Re: [RFC 1/5] USB: core: Add input prompt and help text for USB_OTG config

[Greg KH]

On Wed, Dec 15, 2010 at 04:44:10PM +0530, Pavankumar Kondeti wrote:
> bd6882 commit removes the duplicate USB_OTG config from
> gadget/Kconfig.  But does not copy the input prompt and
> help text to the original config defined in core/Kconfig.
> Add them now.
> 
> Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>

This looks valid to be applied, now, right?

thanks,

greg k-h

Re: [RFC 1/5] USB: core: Add input prompt and help text for USB_OTG config

[Pavan Kondeti]

On 12/16/2010 3:22 AM, Greg KH wrote:
> On Wed, Dec 15, 2010 at 04:44:10PM +0530, Pavankumar Kondeti wrote:
>> bd6882 commit removes the duplicate USB_OTG config from
>> gadget/Kconfig.  But does not copy the input prompt and
>> help text to the original config defined in core/Kconfig.
>> Add them now.
>>
>> Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
> 
> This looks valid to be applied, now, right?
> 
>
Yes. It can be applied now.

I will fix the commit Sergei's comment about commit description and
resend the patch.

Thanks,
Pavan

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[RFC 2/5] USB: core: OTG Supplement Revision 2.0 updates

[Pavankumar Kondeti]

OTG supplement revision 2.0 spec introduces Attach Detection Protocol
(ADP) for detecting peripheral connection without applying power on
VBUS.  ADP is optional and is included in the OTG descriptor along with
SRP and HNP.

HNP polling is introduced for peripheral to notify its wish to become
host.  Host polls (GET_STATUS on DEVICE) peripheral for host_request
and suspend the bus when peripheral returns host_request TRUE.  The spec
insists the polling frequency to be in 1-2 sec range and bus should be
suspended with in 2 sec from host_request is set.

a_alt_hnp_support feature is obsolete and a_hnp_support feature is limited
to only legacy OTG B-device.  The newly introduced bcdOTG field in the OTG
descriptor is used for identifying the 2.0 compliant B-device.

Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
---
 drivers/usb/core/driver.c |   50 +++++++++++++++++++++++++++++++
 drivers/usb/core/hcd.c    |    3 ++
 drivers/usb/core/hub.c    |   71 ++++++++++++++++++++++++++++++++++++++------
 drivers/usb/core/usb.h    |    4 ++
 include/linux/usb.h       |    2 +
 include/linux/usb/ch9.h   |   11 ++++++-
 6 files changed, 129 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index b9278a1..38885b6 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1207,6 +1207,19 @@ static int usb_suspend_both(struct usb_device *udev, pm_message_t msg)
 	 * and flush any outstanding URBs.
 	 */
 	} else {
+#ifdef CONFIG_USB_OTG
+		/* According to OTG supplement Rev 2.0 section 6.3
+		 * Unless an A-device enables b_hnp_enable before entering
+		 * suspend it shall also continue polling while the bus is
+		 * suspended.
+		 *
+		 * We don't have to perform HNP polling, as we are going to
+		 * enable b_hnp_enable before suspending.
+		 */
+		if (udev->bus->hnp_support &&
+			udev->portnum == udev->bus->otg_port)
+			cancel_delayed_work(&udev->bus->hnp_polling);
+#endif
 		udev->can_submit = 0;
 		for (i = 0; i < 16; ++i) {
 			usb_hcd_flush_endpoint(udev, udev->ep_out[i]);
@@ -1270,6 +1283,43 @@ static int usb_resume_both(struct usb_device *udev, pm_message_t msg)
 	return status;
 }
 
+#ifdef CONFIG_USB_OTG
+void usb_hnp_polling_work(struct work_struct *work)
+{
+	int ret;
+	struct usb_bus *bus =
+		container_of(work, struct usb_bus, hnp_polling.work);
+	struct usb_device *udev = bus->root_hub->children[bus->otg_port - 1];
+	u8 *status = kmalloc(sizeof(*status), GFP_KERNEL);
+
+	if (!status)
+		return;
+
+	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+		USB_REQ_GET_STATUS, USB_DIR_IN | USB_RECIP_DEVICE,
+		0, OTG_STATUS_SELECTOR, status, sizeof(*status),
+		USB_CTRL_GET_TIMEOUT);
+	if (ret < 0) {
+		/* Peripheral may not be supporting HNP polling */
+		dev_vdbg(&udev->dev, "HNP polling failed. status %d\n", ret);
+		goto out;
+	}
+
+	/* Spec says host must suspend the bus with in 2 sec. */
+	if (*status & (1 << HOST_REQUEST_FLAG)) {
+		do_unbind_rebind(udev, DO_UNBIND);
+		ret = usb_suspend_both(udev, PMSG_USER_SUSPEND);
+		if (ret)
+			dev_info(&udev->dev, "suspend failed\n");
+	} else {
+		schedule_delayed_work(&bus->hnp_polling,
+			msecs_to_jiffies(THOST_REQ_POLL));
+	}
+out:
+	kfree(status);
+}
+#endif
+
 static void choose_wakeup(struct usb_device *udev, pm_message_t msg)
 {
 	int	w;
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index e70aeaf..6bffd50 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -869,6 +869,9 @@ static void usb_bus_init (struct usb_bus *bus)
 	bus->bandwidth_isoc_reqs = 0;
 
 	INIT_LIST_HEAD (&bus->bus_list);
+#ifdef CONFIG_USB_OTG
+	INIT_DELAYED_WORK(&bus->hnp_polling, usb_hnp_polling_work);
+#endif
 }
 
 /*-------------------------------------------------------------------------*/
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index b98efae..60705a1 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1581,6 +1581,13 @@ void usb_disconnect(struct usb_device **pdev)
 	usb_set_device_state(udev, USB_STATE_NOTATTACHED);
 	dev_info (&udev->dev, "USB disconnect, address %d\n", udev->devnum);
 
+#ifdef CONFIG_USB_OTG
+	if (udev->bus->hnp_support && udev->portnum == udev->bus->otg_port) {
+		cancel_delayed_work_sync(&udev->bus->hnp_polling);
+		udev->bus->hnp_support = 0;
+	}
+#endif
+
 	usb_lock_device(udev);
 
 	/* Free up all the children before we remove this device */
@@ -1685,15 +1692,31 @@ static int usb_enumerate_device_otg(struct usb_device *udev)
 					(port1 == bus->otg_port)
 						? "" : "non-");
 
-				/* enable HNP before suspend, it's simpler */
-				if (port1 == bus->otg_port)
-					bus->b_hnp_enable = 1;
+				/* a_alt_hnp_support is obsoleted */
+				if (port1 != bus->otg_port)
+					goto fail;
+
+				bus->hnp_support = 1;
+
+				/* a_hnp_support is not required for devices
+				 * compliant to revision 2.0 or subsequent
+				 * versions.
+				 */
+				if (desc->bLength == sizeof(*desc) &&
+					le16_to_cpu(desc->bcdOTG) >= 0x0200)
+					goto out;
+
+				/* Legacy B-device i.e compliant to spec
+				 * revision 1.3 expect A-device to set
+				 * a_hnp_support or b_hnp_enable before
+				 * selecting configuration. b_hnp_enable
+				 * is set before suspending the port.
+				 */
+
 				err = usb_control_msg(udev,
 					usb_sndctrlpipe(udev, 0),
 					USB_REQ_SET_FEATURE, 0,
-					bus->b_hnp_enable
-						? USB_DEVICE_B_HNP_ENABLE
-						: USB_DEVICE_A_ALT_HNP_SUPPORT,
+					USB_DEVICE_A_HNP_SUPPORT,
 					0, NULL, 0, USB_CTRL_SET_TIMEOUT);
 				if (err < 0) {
 					/* OTG MESSAGE: report errors here,
@@ -1702,24 +1725,32 @@ static int usb_enumerate_device_otg(struct usb_device *udev)
 					dev_info(&udev->dev,
 						"can't set HNP mode: %d\n",
 						err);
-					bus->b_hnp_enable = 0;
+					bus->hnp_support = 0;
 				}
 			}
 		}
 	}
-
+out:
 	if (!is_targeted(udev)) {
 
 		/* Maybe it can talk to us, though we can't talk to it.
 		 * (Includes HNP test device.)
 		 */
-		if (udev->bus->b_hnp_enable || udev->bus->is_b_host) {
+		if (udev->bus->hnp_support) {
 			err = usb_port_suspend(udev, PMSG_SUSPEND);
 			if (err < 0)
 				dev_dbg(&udev->dev, "HNP fail, %d\n", err);
 		}
 		err = -ENOTSUPP;
-		goto fail;
+	} else if (udev->bus->hnp_support &&
+			udev->portnum == udev->bus->otg_port) {
+		/* HNP polling is introduced in OTG supplement Rev 2.0
+		 * and older devices does not support. Work is not
+		 * re-armed if device returns STALL. B-Host also performs
+		 * HNP polling.
+		 */
+		schedule_delayed_work(&udev->bus->hnp_polling,
+			msecs_to_jiffies(THOST_REQ_POLL));
 	}
 fail:
 #endif
@@ -2210,6 +2241,21 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 		}
 	}
 
+#ifdef CONFIG_USB_OTG
+	if (!udev->bus->is_b_host && udev->bus->hnp_support &&
+			udev->portnum == udev->bus->otg_port) {
+		status = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+				USB_REQ_SET_FEATURE, 0,
+				USB_DEVICE_B_HNP_ENABLE,
+				0, NULL, 0, USB_CTRL_SET_TIMEOUT);
+		if (status < 0)
+			dev_dbg(&udev->dev, "can't enable HNP on port %d, "
+					"status %d\n", port1, status);
+		else
+			udev->bus->b_hnp_enable = 1;
+	}
+#endif
+
 	/* see 7.1.7.6 */
 	status = set_port_feature(hub->hdev, port1, USB_PORT_FEAT_SUSPEND);
 	if (status) {
@@ -2353,6 +2399,11 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 	int		status;
 	u16		portchange, portstatus;
 
+#ifdef CONFIG_USB_OTG
+	if (!udev->bus->is_b_host && udev->bus->hnp_support &&
+			udev->portnum == udev->bus->otg_port)
+		udev->bus->b_hnp_enable = 0;
+#endif
 	/* Skip the initial Clear-Suspend step for a remote wakeup */
 	status = hub_port_status(hub, port1, &portstatus, &portchange);
 	if (status == 0 && !(portstatus & USB_PORT_STAT_SUSPEND))
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index b975450..cd3cb45 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -72,6 +72,10 @@ static inline int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 
 #endif
 
+#ifdef CONFIG_USB_OTG
+extern void usb_hnp_polling_work(struct work_struct *work);
+#endif
+
 #ifdef CONFIG_USB_SUSPEND
 
 extern void usb_autosuspend_device(struct usb_device *udev);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 5ee2223..4ddb8d1 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -317,6 +317,8 @@ struct usb_bus {
 	u8 otg_port;			/* 0, or number of OTG/HNP port */
 	unsigned is_b_host:1;		/* true during some HNP roleswitches */
 	unsigned b_hnp_enable:1;	/* OTG: did A-Host enable HNP? */
+	unsigned hnp_support:1;         /* OTG: HNP is supported on OTG port */
+	struct delayed_work hnp_polling;/* OTG: HNP polling work */
 	unsigned sg_tablesize;		/* 0 or largest number of sg list entries */
 
 	int devnum_next;		/* Next open device number in
diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
index ab46194..504f8b5 100644
--- a/include/linux/usb/ch9.h
+++ b/include/linux/usb/ch9.h
@@ -151,6 +151,11 @@
 #define USB_DEV_STAT_U2_ENABLED		3	/* transition into U2 state */
 #define USB_DEV_STAT_LTM_ENABLED	4	/* Latency tolerance messages */
 
+/* OTG 2.0 spec 6.2 and 6.3 sections */
+#define OTG_STATUS_SELECTOR		0xF000
+#define THOST_REQ_POLL			1500    /* 1000 - 2000 msec */
+#define HOST_REQUEST_FLAG		0
+
 /**
  * struct usb_ctrlrequest - SETUP data for a USB device control request
  * @bRequestType: matches the USB bmRequestType field
@@ -605,17 +610,19 @@ struct usb_qualifier_descriptor {
 
 /*-------------------------------------------------------------------------*/
 
-/* USB_DT_OTG (from OTG 1.0a supplement) */
+/* USB_DT_OTG (from OTG 2.0 supplement) */
 struct usb_otg_descriptor {
 	__u8  bLength;
 	__u8  bDescriptorType;
 
-	__u8  bmAttributes;	/* support for HNP, SRP, etc */
+	__u8  bmAttributes;	/* support for HNP, SRP, ADP etc */
+	__le16 bcdOTG;
 } __attribute__ ((packed));
 
 /* from usb_otg_descriptor.bmAttributes */
 #define USB_OTG_SRP		(1 << 0)
 #define USB_OTG_HNP		(1 << 1)	/* swap host/device roles */
+#define USB_OTG_ADP		(1 << 2)	/* Attach detection protocol */
 
 /*-------------------------------------------------------------------------*/
 
-- 
1.7.1

--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [RFC 2/5] USB: core: OTG Supplement Revision 2.0 updates

[Felipe Balbi]

Hi,

On Wed, Dec 15, 2010 at 04:44:11PM +0530, Pavankumar Kondeti wrote:
>OTG supplement revision 2.0 spec introduces Attach Detection Protocol
>(ADP) for detecting peripheral connection without applying power on
>VBUS.  ADP is optional and is included in the OTG descriptor along with
>SRP and HNP.
>
>HNP polling is introduced for peripheral to notify its wish to become
>host.  Host polls (GET_STATUS on DEVICE) peripheral for host_request
>and suspend the bus when peripheral returns host_request TRUE.  The spec
>insists the polling frequency to be in 1-2 sec range and bus should be
>suspended with in 2 sec from host_request is set.

within ?

>diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
>index b9278a1..38885b6 100644
>--- a/drivers/usb/core/driver.c
>+++ b/drivers/usb/core/driver.c
>@@ -1207,6 +1207,19 @@ static int usb_suspend_both(struct usb_device *udev, pm_message_t msg)
> 	 * and flush any outstanding URBs.
> 	 */
> 	} else {
>+#ifdef CONFIG_USB_OTG
>+		/* According to OTG supplement Rev 2.0 section 6.3
>+		 * Unless an A-device enables b_hnp_enable before entering
>+		 * suspend it shall also continue polling while the bus is
>+		 * suspended.
>+		 *
>+		 * We don't have to perform HNP polling, as we are going to
>+		 * enable b_hnp_enable before suspending.
>+		 */
>+		if (udev->bus->hnp_support &&
>+			udev->portnum == udev->bus->otg_port)
>+			cancel_delayed_work(&udev->bus->hnp_polling);
>+#endif
> 		udev->can_submit = 0;
> 		for (i = 0; i < 16; ++i) {
> 			usb_hcd_flush_endpoint(udev, udev->ep_out[i]);
>@@ -1270,6 +1283,43 @@ static int usb_resume_both(struct usb_device *udev, pm_message_t msg)
> 	return status;
> }
>
>+#ifdef CONFIG_USB_OTG
>+void usb_hnp_polling_work(struct work_struct *work)

do you really need this to be exported ??

>+{
>+	int ret;
>+	struct usb_bus *bus =
>+		container_of(work, struct usb_bus, hnp_polling.work);
>+	struct usb_device *udev = bus->root_hub->children[bus->otg_port - 1];
>+	u8 *status = kmalloc(sizeof(*status), GFP_KERNEL);

how about:

u8 status;

and...

>+	if (!status)
>+		return;
>+
>+	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>+		USB_REQ_GET_STATUS, USB_DIR_IN | USB_RECIP_DEVICE,
>+		0, OTG_STATUS_SELECTOR, status, sizeof(*status),

0, OTG_STATUS_SELECTOR, &status, sizeof(status);
??

-- 
balbi

Re: [RFC 2/5] USB: core: OTG Supplement Revision 2.0 updates

[Alan Stern]

On Wed, 15 Dec 2010, Felipe Balbi wrote:

> >+{
> >+	int ret;
> >+	struct usb_bus *bus =
> >+		container_of(work, struct usb_bus, hnp_polling.work);
> >+	struct usb_device *udev = bus->root_hub->children[bus->otg_port - 1];
> >+	u8 *status = kmalloc(sizeof(*status), GFP_KERNEL);
> 
> how about:
> 
> u8 status;
> 
> and...
> 
> >+	if (!status)
> >+		return;
> >+
> >+	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> >+		USB_REQ_GET_STATUS, USB_DIR_IN | USB_RECIP_DEVICE,
> >+		0, OTG_STATUS_SELECTOR, status, sizeof(*status),
> 
> 0, OTG_STATUS_SELECTOR, &status, sizeof(status);
> ??

You mustn't do DMA to addresses on the stack.

Alan Stern

Re: [RFC 2/5] USB: core: OTG Supplement Revision 2.0 updates

[Felipe Balbi]

On Wed, Dec 15, 2010 at 04:39:13PM -0500, Alan Stern wrote:
>On Wed, 15 Dec 2010, Felipe Balbi wrote:
>
>> >+{
>> >+	int ret;
>> >+	struct usb_bus *bus =
>> >+		container_of(work, struct usb_bus, hnp_polling.work);
>> >+	struct usb_device *udev = bus->root_hub->children[bus->otg_port - 1];
>> >+	u8 *status = kmalloc(sizeof(*status), GFP_KERNEL);
>>
>> how about:
>>
>> u8 status;
>>
>> and...
>>
>> >+	if (!status)
>> >+		return;
>> >+
>> >+	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>> >+		USB_REQ_GET_STATUS, USB_DIR_IN | USB_RECIP_DEVICE,
>> >+		0, OTG_STATUS_SELECTOR, status, sizeof(*status),
>>
>> 0, OTG_STATUS_SELECTOR, &status, sizeof(status);
>> ??
>
>You mustn't do DMA to addresses on the stack.

oh, ok. Forgot other controllers will do DMA for control message too.
MUSB won't.

-- 
balbi

Re: [RFC 2/5] USB: core: OTG Supplement Revision 2.0 updates

[Pavan Kondeti]

On 12/15/2010 5:46 PM, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Dec 15, 2010 at 04:44:11PM +0530, Pavankumar Kondeti wrote:
>> OTG supplement revision 2.0 spec introduces Attach Detection Protocol
>> (ADP) for detecting peripheral connection without applying power on
>> VBUS.  ADP is optional and is included in the OTG descriptor along with
>> SRP and HNP.
>>
>> HNP polling is introduced for peripheral to notify its wish to become
>> host.  Host polls (GET_STATUS on DEVICE) peripheral for host_request
>> and suspend the bus when peripheral returns host_request TRUE.  The spec
>> insists the polling frequency to be in 1-2 sec range and bus should be
>> suspended with in 2 sec from host_request is set.
> 
> within ?
> 
yeah. will fix it in V2.

>> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
>> index b9278a1..38885b6 100644
>> --- a/drivers/usb/core/driver.c
>> +++ b/drivers/usb/core/driver.c
>> @@ -1207,6 +1207,19 @@ static int usb_suspend_both(struct usb_device *udev, pm_message_t msg)
>> 	 * and flush any outstanding URBs.
>> 	 */
>> 	} else {
>> +#ifdef CONFIG_USB_OTG
>> +		/* According to OTG supplement Rev 2.0 section 6.3
>> +		 * Unless an A-device enables b_hnp_enable before entering
>> +		 * suspend it shall also continue polling while the bus is
>> +		 * suspended.
>> +		 *
>> +		 * We don't have to perform HNP polling, as we are going to
>> +		 * enable b_hnp_enable before suspending.
>> +		 */
>> +		if (udev->bus->hnp_support &&
>> +			udev->portnum == udev->bus->otg_port)
>> +			cancel_delayed_work(&udev->bus->hnp_polling);
>> +#endif
>> 		udev->can_submit = 0;
>> 		for (i = 0; i < 16; ++i) {
>> 			usb_hcd_flush_endpoint(udev, udev->ep_out[i]);
>> @@ -1270,6 +1283,43 @@ static int usb_resume_both(struct usb_device *udev, pm_message_t msg)
>> 	return status;
>> }
>>
>> +#ifdef CONFIG_USB_OTG
>> +void usb_hnp_polling_work(struct work_struct *work)
> 
> do you really need this to be exported ??
> 

The work struct is per USB bus. The bus is initialized in core/hcd.c. I
kept the work function here as it it belongs to usb core (I mean not HCD
specific) like other USB auto suspend functions.

Thanks,
Pavan

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [RFC 2/5] USB: core: OTG Supplement Revision 2.0 updates

[Felipe Balbi]

Hi,

On Thu, Dec 16, 2010 at 02:24:05PM +0530, Pavan Kondeti wrote:
>>> @@ -1270,6 +1283,43 @@ static int usb_resume_both(struct usb_device *udev, pm_message_t msg)
>>> 	return status;
>>> }
>>>
>>> +#ifdef CONFIG_USB_OTG
>>> +void usb_hnp_polling_work(struct work_struct *work)
>>
>> do you really need this to be exported ??
>>
>
>The work struct is per USB bus. The bus is initialized in core/hcd.c. I
>kept the work function here as it it belongs to usb core (I mean not HCD
>specific) like other USB auto suspend functions.

I see, it wasn't obvious from the patch, sorry :-)

-- 
balbi

Re: [RFC 2/5] USB: core: OTG Supplement Revision 2.0 updates

[Alan Stern]

On Wed, 15 Dec 2010, Pavankumar Kondeti wrote:

> OTG supplement revision 2.0 spec introduces Attach Detection Protocol
> (ADP) for detecting peripheral connection without applying power on
> VBUS.  ADP is optional and is included in the OTG descriptor along with
> SRP and HNP.
> 
> HNP polling is introduced for peripheral to notify its wish to become
> host.  Host polls (GET_STATUS on DEVICE) peripheral for host_request
> and suspend the bus when peripheral returns host_request TRUE.  The spec
> insists the polling frequency to be in 1-2 sec range and bus should be
> suspended with in 2 sec from host_request is set.
> 
> a_alt_hnp_support feature is obsolete and a_hnp_support feature is limited
> to only legacy OTG B-device.  The newly introduced bcdOTG field in the OTG
> descriptor is used for identifying the 2.0 compliant B-device.
> 
> Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
> ---
>  drivers/usb/core/driver.c |   50 +++++++++++++++++++++++++++++++
>  drivers/usb/core/hcd.c    |    3 ++
>  drivers/usb/core/hub.c    |   71 ++++++++++++++++++++++++++++++++++++++------
>  drivers/usb/core/usb.h    |    4 ++
>  include/linux/usb.h       |    2 +
>  include/linux/usb/ch9.h   |   11 ++++++-
>  6 files changed, 129 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index b9278a1..38885b6 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c

> @@ -1270,6 +1283,43 @@ static int usb_resume_both(struct usb_device *udev, pm_message_t msg)
>  	return status;
>  }
>  
> +#ifdef CONFIG_USB_OTG
> +void usb_hnp_polling_work(struct work_struct *work)
> +{
> +	int ret;
> +	struct usb_bus *bus =
> +		container_of(work, struct usb_bus, hnp_polling.work);
> +	struct usb_device *udev = bus->root_hub->children[bus->otg_port - 1];
> +	u8 *status = kmalloc(sizeof(*status), GFP_KERNEL);
> +
> +	if (!status)
> +		return;

Shouldn't you reschedule the delayed work?  A memory allocation failure 
is likely to be temporary.

> +
> +	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> +		USB_REQ_GET_STATUS, USB_DIR_IN | USB_RECIP_DEVICE,
> +		0, OTG_STATUS_SELECTOR, status, sizeof(*status),
> +		USB_CTRL_GET_TIMEOUT);
> +	if (ret < 0) {
> +		/* Peripheral may not be supporting HNP polling */
> +		dev_vdbg(&udev->dev, "HNP polling failed. status %d\n", ret);
> +		goto out;
> +	}
> +
> +	/* Spec says host must suspend the bus with in 2 sec. */
> +	if (*status & (1 << HOST_REQUEST_FLAG)) {
> +		do_unbind_rebind(udev, DO_UNBIND);

You forget to set udev->do_remote_wakeup to 0.

> +		ret = usb_suspend_both(udev, PMSG_USER_SUSPEND);
> +		if (ret)
> +			dev_info(&udev->dev, "suspend failed\n");
> +	} else {
> +		schedule_delayed_work(&bus->hnp_polling,
> +			msecs_to_jiffies(THOST_REQ_POLL));
> +	}
> +out:
> +	kfree(status);
> +}
> +#endif

Alan Stern

Re: [RFC 2/5] USB: core: OTG Supplement Revision 2.0 updates

[Pavan Kondeti]

On 12/16/2010 2:59 AM, Alan Stern wrote:
> On Wed, 15 Dec 2010, Pavankumar Kondeti wrote:
> 
>> OTG supplement revision 2.0 spec introduces Attach Detection Protocol
>> (ADP) for detecting peripheral connection without applying power on
>> VBUS.  ADP is optional and is included in the OTG descriptor along with
>> SRP and HNP.
>>
>> HNP polling is introduced for peripheral to notify its wish to become
>> host.  Host polls (GET_STATUS on DEVICE) peripheral for host_request
>> and suspend the bus when peripheral returns host_request TRUE.  The spec
>> insists the polling frequency to be in 1-2 sec range and bus should be
>> suspended with in 2 sec from host_request is set.
>>
>> a_alt_hnp_support feature is obsolete and a_hnp_support feature is limited
>> to only legacy OTG B-device.  The newly introduced bcdOTG field in the OTG
>> descriptor is used for identifying the 2.0 compliant B-device.
>>
>> Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
>> ---
>>  drivers/usb/core/driver.c |   50 +++++++++++++++++++++++++++++++
>>  drivers/usb/core/hcd.c    |    3 ++
>>  drivers/usb/core/hub.c    |   71 ++++++++++++++++++++++++++++++++++++++------
>>  drivers/usb/core/usb.h    |    4 ++
>>  include/linux/usb.h       |    2 +
>>  include/linux/usb/ch9.h   |   11 ++++++-
>>  6 files changed, 129 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
>> index b9278a1..38885b6 100644
>> --- a/drivers/usb/core/driver.c
>> +++ b/drivers/usb/core/driver.c
> 
>> @@ -1270,6 +1283,43 @@ static int usb_resume_both(struct usb_device *udev, pm_message_t msg)
>>  	return status;
>>  }
>>  
>> +#ifdef CONFIG_USB_OTG
>> +void usb_hnp_polling_work(struct work_struct *work)
>> +{
>> +	int ret;
>> +	struct usb_bus *bus =
>> +		container_of(work, struct usb_bus, hnp_polling.work);
>> +	struct usb_device *udev = bus->root_hub->children[bus->otg_port - 1];
>> +	u8 *status = kmalloc(sizeof(*status), GFP_KERNEL);
>> +
>> +	if (!status)
>> +		return;
> 
> Shouldn't you reschedule the delayed work?  A memory allocation failure 
> is likely to be temporary.
> 
Agreed. Will fix it in V2.

>> +
>> +	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>> +		USB_REQ_GET_STATUS, USB_DIR_IN | USB_RECIP_DEVICE,
>> +		0, OTG_STATUS_SELECTOR, status, sizeof(*status),
>> +		USB_CTRL_GET_TIMEOUT);
>> +	if (ret < 0) {
>> +		/* Peripheral may not be supporting HNP polling */
>> +		dev_vdbg(&udev->dev, "HNP polling failed. status %d\n", ret);
>> +		goto out;
>> +	}
>> +
>> +	/* Spec says host must suspend the bus with in 2 sec. */
>> +	if (*status & (1 << HOST_REQUEST_FLAG)) {
>> +		do_unbind_rebind(udev, DO_UNBIND);
> 
> You forget to set udev->do_remote_wakeup to 0.
> 
Thanks for pointing this out. I will fix it in V2.

Thanks,
Pavan

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [RFC 2/5] USB: core: OTG Supplement Revision 2.0 updates

[Greg KH]

On Wed, Dec 15, 2010 at 04:44:11PM +0530, Pavankumar Kondeti wrote:
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1207,6 +1207,19 @@ static int usb_suspend_both(struct usb_device *udev, pm_message_t msg)
>  	 * and flush any outstanding URBs.
>  	 */
>  	} else {
> +#ifdef CONFIG_USB_OTG

No ifdefs allowed, sorry.

Please rework this.

thanks,

greg k-h

Re: [RFC 2/5] USB: core: OTG Supplement Revision 2.0 updates

[Pavan Kondeti]

On 12/16/2010 3:23 AM, Greg KH wrote:
> On Wed, Dec 15, 2010 at 04:44:11PM +0530, Pavankumar Kondeti wrote:
>> --- a/drivers/usb/core/driver.c
>> +++ b/drivers/usb/core/driver.c
>> @@ -1207,6 +1207,19 @@ static int usb_suspend_both(struct usb_device *udev, pm_message_t msg)
>>  	 * and flush any outstanding URBs.
>>  	 */
>>  	} else {
>> +#ifdef CONFIG_USB_OTG
> 
> No ifdefs allowed, sorry.
> 
> Please rework this.
> 

I have seen using CONFIG_USB_OTG in core/hub.c and followed the same.
But the code I have added does not have to depend on CONFGI_USB_OTG.

I will fix this in V2 patch set.

Thanks,
Pavan

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[RFC 3/5] USB: gadget: OTG supplement revision 2.0 updates

[Pavankumar Kondeti]

Introduce otg_version field in usb_gadget struct.  UDC can
advertise OTG spec version compatibility by setting otg_version
field appropriately.  Gadget drivers fill the bcdOTG field in
OTG descriptor based on UDC's OTG version.

Add sysfs file for host_request and UDC returns the same when
HNP polling request arrives from the host.

Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
---
This patch changes only gadget Zero. But the final patch will
add OTG2.0 support for all the gadget drivers.

 .../testing/sysfs-devices-platform-_UDC_-gadget    |   14 +++++++++++
 drivers/usb/gadget/composite.c                     |   25 +++++++++++++++++++-
 drivers/usb/gadget/zero.c                          |    3 ++
 include/linux/usb/gadget.h                         |   21 ++++++++++++++++
 4 files changed, 62 insertions(+), 1 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-devices-platform-_UDC_-gadget b/Documentation/ABI/testing/sysfs-devices-platform-_UDC_-gadget
index d548eaa..9c622e4 100644
--- a/Documentation/ABI/testing/sysfs-devices-platform-_UDC_-gadget
+++ b/Documentation/ABI/testing/sysfs-devices-platform-_UDC_-gadget
@@ -19,3 +19,17 @@ Description:
 		Possible values are:
 			1 -> ignore the FUA flag
 			0 -> obey the FUA flag
+
+What:		/sys/devices/platform/_UDC_/gadget/host_request
+Date:		December 2010
+Contact:	Pavan Kondeti <pkondeti@codeaurora.org>
+Description:
+		OTG 2.0 compliant host keeps polling OTG2.0 peripheral
+		for host role. Set host_request flag, which tells host
+		to give up the host role to peripheral.
+
+		1 -> host role is requested
+		0 -> no effect (automatically cleared upon reset/disconnect)
+
+		(_UDC_ is the name of the USB Device Controller driver)
+
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 21dc0da..f3a2023 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -978,6 +978,7 @@ static void composite_disconnect(struct usb_gadget *gadget)
 	struct usb_composite_dev	*cdev = get_gadget_data(gadget);
 	unsigned long			flags;
 
+	gadget->host_request = 0;
 	/* REVISIT:  should we have config and device level
 	 * disconnect callbacks?
 	 */
@@ -1003,6 +1004,23 @@ static ssize_t composite_show_suspended(struct device *dev,
 
 static DEVICE_ATTR(suspended, 0444, composite_show_suspended, NULL);
 
+static ssize_t composite_set_host_request(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count)
+{
+	struct usb_gadget *gadget = dev_to_usb_gadget(dev);
+	int value;
+
+	if (sscanf(buf, "%d", &value) != 1)
+		return -EINVAL;
+
+	gadget->host_request = !!value;
+	return count;
+
+}
+
+static DEVICE_ATTR(host_request, S_IWUSR, NULL, composite_set_host_request);
+
 static void
 composite_unbind(struct usb_gadget *gadget)
 {
@@ -1047,9 +1065,10 @@ composite_unbind(struct usb_gadget *gadget)
 		kfree(cdev->req->buf);
 		usb_ep_free_request(gadget->ep0, cdev->req);
 	}
+	device_remove_file(&gadget->dev, &dev_attr_host_request);
+	device_remove_file(&gadget->dev, &dev_attr_suspended);
 	kfree(cdev);
 	set_gadget_data(gadget, NULL);
-	device_remove_file(&gadget->dev, &dev_attr_suspended);
 	composite = NULL;
 }
 
@@ -1158,6 +1177,10 @@ static int composite_bind(struct usb_gadget *gadget)
 	if (status)
 		goto fail;
 
+	status = device_create_file(&gadget->dev, &dev_attr_host_request);
+	if (status)
+		DBG(cdev, "unable to create host_request sysfs file\n");
+
 	INFO(cdev, "%s ready\n", composite->name);
 	return 0;
 
diff --git a/drivers/usb/gadget/zero.c b/drivers/usb/gadget/zero.c
index 6d16db9..b4fb719 100644
--- a/drivers/usb/gadget/zero.c
+++ b/drivers/usb/gadget/zero.c
@@ -293,6 +293,9 @@ static int __init zero_bind(struct usb_composite_dev *cdev)
 
 	setup_timer(&autoresume_timer, zero_autoresume, (unsigned long) cdev);
 
+	if (gadget_is_otg2(cdev->gadget))
+		otg_descriptor.bcdOTG = __constant_cpu_to_le16(0x0200);
+
 	/* Register primary, then secondary configuration.  Note that
 	 * SH3 only allows one config...
 	 */
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 006412c..b891257 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -443,6 +443,8 @@ struct usb_gadget_ops {
  *	operation.  If it does, the gadget driver must also support both.
  * @is_otg: True if the USB device port uses a Mini-AB jack, so that the
  *	gadget driver must provide a USB OTG descriptor.
+ * @otg_version: UDC OTG version based on which gadget driver fills the
+ *	bcdOTG field in a USB OTG descriptor.
  * @is_a_peripheral: False unless is_otg, the "A" end of a USB cable
  *	is in the Mini-AB jack, and HNP has been used to switch roles
  *	so that the "A" device currently acts as A-Peripheral, not A-Host.
@@ -452,6 +454,7 @@ struct usb_gadget_ops {
  *	only supports HNP on a different root port.
  * @b_hnp_enable: OTG device feature flag, indicating that the A-Host
  *	enabled HNP support.
+ * @host_request: A Flag, indicating that user wishes to take the host role.
  * @name: Identifies the controller hardware type.  Used in diagnostics
  *	and sometimes configuration.
  * @dev: Driver model state for this abstract device.
@@ -482,10 +485,14 @@ struct usb_gadget {
 	enum usb_device_speed		speed;
 	unsigned			is_dualspeed:1;
 	unsigned			is_otg:1;
+	u16				otg_version;
+#define	UDC_OTG1	0x0000
+#define	UDC_OTG2	0x0001
 	unsigned			is_a_peripheral:1;
 	unsigned			b_hnp_enable:1;
 	unsigned			a_hnp_support:1;
 	unsigned			a_alt_hnp_support:1;
+	unsigned			host_request:1;
 	const char			*name;
 	struct device			dev;
 };
@@ -537,6 +544,20 @@ static inline int gadget_is_otg(struct usb_gadget *g)
 }
 
 /**
+ * gadget_is_otg2 - return true if UDC is compliant to OTG 2.0
+ * @g: controller that might have a Mini-AB/Micro-AB connector
+ *
+ */
+static inline int gadget_is_otg2(struct usb_gadget *g)
+{
+#ifdef CONFIG_USB_OTG
+	return g->otg_version && UDC_OTG2;
+#else
+	return 0;
+#endif
+}
+
+/**
  * usb_gadget_frame_number - returns the current frame number
  * @gadget: controller that reports the frame number
  *
-- 
1.7.1

--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [RFC 3/5] USB: gadget: OTG supplement revision 2.0 updates

[Felipe Balbi]

Hi,

On Wed, Dec 15, 2010 at 04:44:12PM +0530, Pavankumar Kondeti wrote:
>@@ -1047,9 +1065,10 @@ composite_unbind(struct usb_gadget *gadget)
> 		kfree(cdev->req->buf);
> 		usb_ep_free_request(gadget->ep0, cdev->req);
> 	}
>+	device_remove_file(&gadget->dev, &dev_attr_host_request);
>+	device_remove_file(&gadget->dev, &dev_attr_suspended);
> 	kfree(cdev);
> 	set_gadget_data(gadget, NULL);
>-	device_remove_file(&gadget->dev, &dev_attr_suspended);

this looks like something that shouldn't be part of this patch ?!?

-- 
balbi

Re: [RFC 3/5] USB: gadget: OTG supplement revision 2.0 updates

[Pavan Kondeti]

On 12/15/2010 5:48 PM, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Dec 15, 2010 at 04:44:12PM +0530, Pavankumar Kondeti wrote:
>> @@ -1047,9 +1065,10 @@ composite_unbind(struct usb_gadget *gadget)
>> 		kfree(cdev->req->buf);
>> 		usb_ep_free_request(gadget->ep0, cdev->req);
>> 	}
>> +	device_remove_file(&gadget->dev, &dev_attr_host_request);
>> +	device_remove_file(&gadget->dev, &dev_attr_suspended);
>> 	kfree(cdev);
>> 	set_gadget_data(gadget, NULL);
>> -	device_remove_file(&gadget->dev, &dev_attr_suspended);
> 
> this looks like something that shouldn't be part of this patch ?!?
> 
Yeah. I will send another patch for removing suspended sysfs file
before freeing the cdev.

Thanks,
Pavan

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[RFC 4/5] USB: EHCI: Notify HCD about HNP enabled port suspend

[Pavankumar Kondeti]

Introduce start_hnp callback function for HCD to receive notification
from EHCI core that HNP enabled port is suspended.  HCD may initiate
HNP or notify the same to OTG via otg_start_hnp().

This patch is inspired by "USB: Hook start_hnp into ohci struct"
(e8b24450).

Change-Id: I8e258a6fdf42c166ea9cb3a727e4d3d28a8adc72
Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
---
 drivers/usb/host/ehci-hub.c |   11 +++++++++++
 drivers/usb/host/ehci.h     |    2 ++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 796ea0c..65bf104 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -1018,6 +1018,17 @@ static int ehci_hub_control (
 					|| (temp & PORT_RESET) != 0)
 				goto error;
 
+#ifdef	CONFIG_USB_OTG
+			if (hcd->self.otg_port == (wIndex + 1) &&
+					hcd->self.b_hnp_enable &&
+					ehci->start_hnp) {
+				ehci_writel(ehci, temp | PORT_SUSPEND,
+						status_reg);
+				set_bit(wIndex, &ehci->suspended_ports);
+				ehci->start_hnp(ehci);
+				break;
+			}
+#endif
 			/* After above check the port must be connected.
 			 * Set appropriate bit thus could put phy into low power
 			 * mode if we have hostpc feature
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index fd1c53d..a4989f2 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -122,6 +122,8 @@ struct ehci_hcd {			/* one per controller */
 	ktime_t			last_periodic_enable;
 	u32			command;
 
+	void (*start_hnp)(struct ehci_hcd *ehci);
+
 	/* SILICON QUIRKS */
 	unsigned		no_selective_suspend:1;
 	unsigned		has_fsl_port_bug:1; /* FreeScale */
-- 
1.7.1

--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [RFC 4/5] USB: EHCI: Notify HCD about HNP enabled port suspend

[Felipe Balbi]

Hi,

On Wed, Dec 15, 2010 at 04:44:13PM +0530, Pavankumar Kondeti wrote:
>Introduce start_hnp callback function for HCD to receive notification
>from EHCI core that HNP enabled port is suspended.  HCD may initiate
>HNP or notify the same to OTG via otg_start_hnp().
>
>This patch is inspired by "USB: Hook start_hnp into ohci struct"
>(e8b24450).
>
>Change-Id: I8e258a6fdf42c166ea9cb3a727e4d3d28a8adc72
>Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
>---
> drivers/usb/host/ehci-hub.c |   11 +++++++++++
> drivers/usb/host/ehci.h     |    2 ++
> 2 files changed, 13 insertions(+), 0 deletions(-)
>
>diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
>index 796ea0c..65bf104 100644
>--- a/drivers/usb/host/ehci-hub.c
>+++ b/drivers/usb/host/ehci-hub.c
>@@ -1018,6 +1018,17 @@ static int ehci_hub_control (
> 					|| (temp & PORT_RESET) != 0)
> 				goto error;
>
>+#ifdef	CONFIG_USB_OTG
>+			if (hcd->self.otg_port == (wIndex + 1) &&
>+					hcd->self.b_hnp_enable &&
>+					ehci->start_hnp) {
>+				ehci_writel(ehci, temp | PORT_SUSPEND,
>+						status_reg);
>+				set_bit(wIndex, &ehci->suspended_ports);
>+				ehci->start_hnp(ehci);

should you check if this pointer is valid before ?

-- 
balbi

Re: [RFC 4/5] USB: EHCI: Notify HCD about HNP enabled port suspend

[Pavan Kondeti]

On 12/15/2010 5:49 PM, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Dec 15, 2010 at 04:44:13PM +0530, Pavankumar Kondeti wrote:
>> Introduce start_hnp callback function for HCD to receive notification
>>from EHCI core that HNP enabled port is suspended.  HCD may initiate
>> HNP or notify the same to OTG via otg_start_hnp().
>>
>> This patch is inspired by "USB: Hook start_hnp into ohci struct"
>> (e8b24450).
>>
>> Change-Id: I8e258a6fdf42c166ea9cb3a727e4d3d28a8adc72
>> Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
>> ---
>> drivers/usb/host/ehci-hub.c |   11 +++++++++++
>> drivers/usb/host/ehci.h     |    2 ++
>> 2 files changed, 13 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
>> index 796ea0c..65bf104 100644
>> --- a/drivers/usb/host/ehci-hub.c
>> +++ b/drivers/usb/host/ehci-hub.c
>> @@ -1018,6 +1018,17 @@ static int ehci_hub_control (
>> 					|| (temp & PORT_RESET) != 0)
>> 				goto error;
>>
>> +#ifdef	CONFIG_USB_OTG
>> +			if (hcd->self.otg_port == (wIndex + 1) &&
>> +					hcd->self.b_hnp_enable &&
>> +					ehci->start_hnp) {

The pointer is checked here...

>> +				ehci_writel(ehci, temp | PORT_SUSPEND,
>> +						status_reg);
>> +				set_bit(wIndex, &ehci->suspended_ports);
>> +				ehci->start_hnp(ehci);
> 
> should you check if this pointer is valid before ?
> 


-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[RFC 5/5] USB: Eliminate delays involved in root hub initialization during HNP

[Pavankumar Kondeti]

Some USB controllers have common resources (IRQ, register address
space) for Host, Peripheral and OTG.  So HCD is added only before
entering into Host mode.  Root hub initialization is done in
different steps to decrease boot up time.  But this makes B-device
difficult to meet HNP timings.  Hence eliminate delays involved in
root hub initialization for B-host.

This patch also marks hnp_supported flag TURE for B-host while
registering the bus.

Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
---
 drivers/usb/core/hcd.c |    4 ++++
 drivers/usb/core/hub.c |   13 +++++++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 6bffd50..a180a0f 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -902,6 +902,10 @@ static int usb_register_bus(struct usb_bus *bus)
 	list_add (&bus->bus_list, &usb_bus_list);
 	mutex_unlock(&usb_bus_list_lock);
 
+#ifdef CONFIG_USB_OTG
+	if (bus->is_b_host)
+		bus->hnp_support = 1;
+#endif
 	usb_notify_add_bus(bus);
 
 	dev_info (bus->controller, "new USB bus registered, assigned bus "
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 60705a1..48fb83d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -706,6 +706,10 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 		 */
 		if (type == HUB_INIT) {
 			delay = hub_power_on(hub, false);
+#ifdef CONFIG_USB_OTG
+			if (hdev->bus->is_b_host)
+				goto init2;
+#endif
 			PREPARE_DELAYED_WORK(&hub->init_work, hub_init_func2);
 			schedule_delayed_work(&hub->init_work,
 					msecs_to_jiffies(delay));
@@ -811,6 +815,11 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 		}
 	}
 
+#ifdef CONFIG_USB_OTG
+		if (hdev->bus->is_b_host && type == HUB_INIT)
+			goto init3;
+#endif
+
 	/* If no port-status-change flags were set, we don't need any
 	 * debouncing.  If flags were set we can try to debounce the
 	 * ports all at once right now, instead of letting khubd do them
@@ -844,6 +853,10 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 	/* Scan all ports that need attention */
 	kick_khubd(hub);
 
+#ifdef CONFIG_USB_OTG
+	if (hdev->bus->is_b_host && type == HUB_INIT)
+		return;
+#endif
 	/* Allow autosuspend if it was suppressed */
 	if (type <= HUB_INIT3)
 		usb_autopm_put_interface_async(to_usb_interface(hub->intfdev));
-- 
1.7.1

--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [RFC 5/5] USB: Eliminate delays involved in root hub initialization during HNP

[Felipe Balbi]

On Wed, Dec 15, 2010 at 04:44:14PM +0530, Pavankumar Kondeti wrote:
>Some USB controllers have common resources (IRQ, register address
>space) for Host, Peripheral and OTG.  So HCD is added only before
>entering into Host mode.  Root hub initialization is done in
>different steps to decrease boot up time.  But this makes B-device
>difficult to meet HNP timings.  Hence eliminate delays involved in
>root hub initialization for B-host.

I wonder if this is the best approach. Would it be easier to not touch
usbcore, probe the entire stack during boot but have a "core" layer
handling synchronization to shared resources ?

Maybe you could make your device an MFD device that allocates
platform_devices for its children (HCI, UDC, OTG, etc) and pass in a
bunch of read/write functions as platform_data for them to use as
accessors to shared register space ?!? Would that work ?

>This patch also marks hnp_supported flag TURE for B-host while

s/TURE/TRUE

-- 
balbi

Re: [RFC 5/5] USB: Eliminate delays involved in root hub initialization during HNP

[Pavan Kondeti]

On 12/15/2010 5:51 PM, Felipe Balbi wrote:
> On Wed, Dec 15, 2010 at 04:44:14PM +0530, Pavankumar Kondeti wrote:
>> Some USB controllers have common resources (IRQ, register address
>> space) for Host, Peripheral and OTG.  So HCD is added only before
>> entering into Host mode.  Root hub initialization is done in
>> different steps to decrease boot up time.  But this makes B-device
>> difficult to meet HNP timings.  Hence eliminate delays involved in
>> root hub initialization for B-host.
> 
> I wonder if this is the best approach. Would it be easier to not touch
> usbcore, probe the entire stack during boot but have a "core" layer
> handling synchronization to shared resources ?
> 
The implementation is like this:

Actually OTG synchronizes UDC and HCD. OTG driver probe will be called
first and it takes care of turning on clocks, resetting controller and
PHY and enabling interrupts (VBUS and Id). UDC and HCD will not modify
hardware registers in their probe functions. So HCD will not call
usb_add_hcd() function in it's probe(). After HCD and UDC registers with
OTG via otg_set_xxx(), OTG activates HCD (by calling usb_add_hcd()) or
UDC (usb_gadget_connect()) based on Id/VBUS status. When gadget is
active, HCD is detached from USB core (usb_remove_hcd()). So user space
can not poke into sysfs/debugfs nodes provided in the usb core.

> Maybe you could make your device an MFD device that allocates
> platform_devices for its children (HCI, UDC, OTG, etc) and pass in a
> bunch of read/write functions as platform_data for them to use as
> accessors to shared register space ?!? Would that work ?
> 
The easy way to deal this problem is removing HCD when Mini/Micro -A
cable is detached.

This patch addresses the issues (usb_add_hcd() taking too much time
)when B-peripheral becomes B-host.

>> This patch also marks hnp_supported flag TURE for B-host while
> 
> s/TURE/TRUE
> 
Fixed in V2.

Thanks,
Pavan

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [RFC 5/5] USB: Eliminate delays involved in root hub initialization during HNP

[Felipe Balbi]

On Thu, Dec 16, 2010 at 04:56:15PM +0530, Pavan Kondeti wrote:
>On 12/15/2010 5:51 PM, Felipe Balbi wrote:
>> On Wed, Dec 15, 2010 at 04:44:14PM +0530, Pavankumar Kondeti wrote:
>>> Some USB controllers have common resources (IRQ, register address
>>> space) for Host, Peripheral and OTG.  So HCD is added only before
>>> entering into Host mode.  Root hub initialization is done in
>>> different steps to decrease boot up time.  But this makes B-device
>>> difficult to meet HNP timings.  Hence eliminate delays involved in
>>> root hub initialization for B-host.
>>
>> I wonder if this is the best approach. Would it be easier to not touch
>> usbcore, probe the entire stack during boot but have a "core" layer
>> handling synchronization to shared resources ?
>>
>The implementation is like this:
>
>Actually OTG synchronizes UDC and HCD. OTG driver probe will be called
>first and it takes care of turning on clocks, resetting controller and
>PHY and enabling interrupts (VBUS and Id). UDC and HCD will not modify
>hardware registers in their probe functions. So HCD will not call
>usb_add_hcd() function in it's probe(). After HCD and UDC registers with
>OTG via otg_set_xxx(), OTG activates HCD (by calling usb_add_hcd()) or
>UDC (usb_gadget_connect()) based on Id/VBUS status. When gadget is
>active, HCD is detached from USB core (usb_remove_hcd()). So user space
>can not poke into sysfs/debugfs nodes provided in the usb core.
>
>> Maybe you could make your device an MFD device that allocates
>> platform_devices for its children (HCI, UDC, OTG, etc) and pass in a
>> bunch of read/write functions as platform_data for them to use as
>> accessors to shared register space ?!? Would that work ?
>>
>The easy way to deal this problem is removing HCD when Mini/Micro -A
>cable is detached.

But why do you need to do that isn't clear. What's the problem in
keeping HCD always available ? If it's a mini/micro-B cable you will
enter peripheral role anyway.

-- 
balbi

Re: [RFC 5/5] USB: Eliminate delays involved in root hub initialization during HNP

[Pavan Kondeti]

On 12/16/2010 5:45 PM, Felipe Balbi wrote:
> On Thu, Dec 16, 2010 at 04:56:15PM +0530, Pavan Kondeti wrote:
>> On 12/15/2010 5:51 PM, Felipe Balbi wrote:
>>> On Wed, Dec 15, 2010 at 04:44:14PM +0530, Pavankumar Kondeti wrote:
>>>> Some USB controllers have common resources (IRQ, register address
>>>> space) for Host, Peripheral and OTG.  So HCD is added only before
>>>> entering into Host mode.  Root hub initialization is done in
>>>> different steps to decrease boot up time.  But this makes B-device
>>>> difficult to meet HNP timings.  Hence eliminate delays involved in
>>>> root hub initialization for B-host.
>>>
>>> I wonder if this is the best approach. Would it be easier to not touch
>>> usbcore, probe the entire stack during boot but have a "core" layer
>>> handling synchronization to shared resources ?
>>>
>> The implementation is like this:
>>
>> Actually OTG synchronizes UDC and HCD. OTG driver probe will be called
>> first and it takes care of turning on clocks, resetting controller and
>> PHY and enabling interrupts (VBUS and Id). UDC and HCD will not modify
>> hardware registers in their probe functions. So HCD will not call
>> usb_add_hcd() function in it's probe(). After HCD and UDC registers with
>> OTG via otg_set_xxx(), OTG activates HCD (by calling usb_add_hcd()) or
>> UDC (usb_gadget_connect()) based on Id/VBUS status. When gadget is
>> active, HCD is detached from USB core (usb_remove_hcd()). So user space
>> can not poke into sysfs/debugfs nodes provided in the usb core.
>>
>>> Maybe you could make your device an MFD device that allocates
>>> platform_devices for its children (HCI, UDC, OTG, etc) and pass in a
>>> bunch of read/write functions as platform_data for them to use as
>>> accessors to shared register space ?!? Would that work ?
>>>
>> The easy way to deal this problem is removing HCD when Mini/Micro -A
>> cable is detached.
> 
> But why do you need to do that isn't clear. What's the problem in
> keeping HCD always available ? If it's a mini/micro-B cable you will
> enter peripheral role anyway.
> 
If you keep the HCD, root hub auto/system suspend can happen, user may
un-configure the root hub or remove the HCD etc. There is not standard
way of guarding against PM & user for this kind of HCD.

We had a discussion about this some time back.

Please see http://www.spinics.net/lists/linux-usb/msg35847.html

Thanks,
Pavan

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [RFC 5/5] USB: Eliminate delays involved in root hub initialization during HNP

[Greg KH]

On Wed, Dec 15, 2010 at 04:44:14PM +0530, Pavankumar Kondeti wrote:
> Some USB controllers have common resources (IRQ, register address
> space) for Host, Peripheral and OTG.  So HCD is added only before
> entering into Host mode.  Root hub initialization is done in
> different steps to decrease boot up time.  But this makes B-device
> difficult to meet HNP timings.  Hence eliminate delays involved in
> root hub initialization for B-host.
> 
> This patch also marks hnp_supported flag TURE for B-host while
> registering the bus.
> 
> Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
> ---
>  drivers/usb/core/hcd.c |    4 ++++
>  drivers/usb/core/hub.c |   13 +++++++++++++
>  2 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 6bffd50..a180a0f 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -902,6 +902,10 @@ static int usb_register_bus(struct usb_bus *bus)
>  	list_add (&bus->bus_list, &usb_bus_list);
>  	mutex_unlock(&usb_bus_list_lock);
>  
> +#ifdef CONFIG_USB_OTG
> +	if (bus->is_b_host)
> +		bus->hnp_support = 1;
> +#endif
>  	usb_notify_add_bus(bus);
>  
>  	dev_info (bus->controller, "new USB bus registered, assigned bus "
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 60705a1..48fb83d 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -706,6 +706,10 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
>  		 */
>  		if (type == HUB_INIT) {
>  			delay = hub_power_on(hub, false);
> +#ifdef CONFIG_USB_OTG
> +			if (hdev->bus->is_b_host)
> +				goto init2;
> +#endif

Never have #ifdefs in .c code, that is totally unacceptable and
unworkable for stuff like this.

Come on, you know better than this...

thanks,

greg k-h

Re: [RFC 0/5] USB core changes for supporting OTG on MSM SoC

[Alan Stern]

On Wed, 15 Dec 2010, Pavankumar Kondeti wrote:

> This patch series adds OTG 2.0 enhancements and misc changes
> required for supporting SRP & HNP in MSM OTG driver.  As these
> patches modify the USB core code, I would like to receive
> feedback before posting driver patches. The driver patches are
> available at
> https://www.codeaurora.org/gitweb/quic/la/?p=kernel/msm.git;a=commitdiff;h=05535a995e9eb34c3c41d874d09955a443008cd5

All those "#ifdef CONFIG_USB_OTG" lines you are sprinkling throughout 
the core are very annoying.  A few simple inline routines should allow 
you to eliminate many of them.

Also, in the 5/5 patch in hub_activate(), all those

	if (hdev->bus->is_b_host && type == HUB_INIT)

tests are distracting.  You can make the test once, at the start of the 
function, and store the result in a local variable.

Alan Stern

Re: [RFC 0/5] USB core changes for supporting OTG on MSM SoC

[Pavan Kondeti]

On 12/16/2010 3:08 AM, Alan Stern wrote:
> On Wed, 15 Dec 2010, Pavankumar Kondeti wrote:
> 
>> This patch series adds OTG 2.0 enhancements and misc changes
>> required for supporting SRP & HNP in MSM OTG driver.  As these
>> patches modify the USB core code, I would like to receive
>> feedback before posting driver patches. The driver patches are
>> available at
>> https://www.codeaurora.org/gitweb/quic/la/?p=kernel/msm.git;a=commitdiff;h=05535a995e9eb34c3c41d874d09955a443008cd5
> 
> All those "#ifdef CONFIG_USB_OTG" lines you are sprinkling throughout 
> the core are very annoying.  A few simple inline routines should allow 
> you to eliminate many of them.
> 
I am doing OTG specific operations only after checking bus->hnp_support,
udev->port == bus->otg_port, which will be true only for OTG device and
CONFIG_USB_OTG is enabled. So I guess I can put them directly without
#ifdef.

> Also, in the 5/5 patch in hub_activate(), all those
> 
> 	if (hdev->bus->is_b_host && type == HUB_INIT)
> 
> tests are distracting.  You can make the test once, at the start of the 
> function, and store the result in a local variable.
> 
Okay. I will fix this in V2.

Thanks,
Pavan

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.