Re: [PATCH v2 05/22] doc: dt-binding: usb: add otg related properties

[Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>]

On Fri, 12 Jun 2015 16:23:59 +0800
Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> On Fri, Jun 12, 2015 at 11:02:13AM +0300, Roger Quadros wrote:
> > 
> > On Fri, 12 Jun 2015 09:42:04 +0800
> > Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > 
> > > On Thu, Jun 11, 2015 at 05:55:57PM +0300, Roger Quadros wrote:
> > > > > > drivers/usb/core/hub.c
> > > > > > 
> > > > > > static int usb_enumerate_device_otg(struct usb_device *udev)
> > > > > > {
> > > > > > 	int err = 0;
> > > > > > 
> > > > > > #ifdef	CONFIG_USB_OTG
> > > > > > 	/*
> > > > > > 	 * OTG-aware devices on OTG-capable root hubs may be able to use SRP,
> > > > > > 	 * to wake us after we've powered off VBUS; and HNP, switching roles
> > > > > > 	 * "host" to "peripheral".  The OTG descriptor helps figure this out.
> > > > > > 	 */
> > > > > > 	if (!udev->bus->is_b_host
> > > > > > 			&& udev->config
> > > > > > 			&& udev->parent == udev->bus->root_hub) {
> > > > > > 		struct usb_otg_descriptor	*desc = NULL;
> > > > > > 		struct usb_bus			*bus = udev->bus;
> > > > > > 
> > > > > > 		/* descriptor may appear anywhere in config */
> > > > > > 		if (__usb_get_extra_descriptor (udev->rawdescriptors[0],
> > > > > > 					le16_to_cpu(udev->config[0].desc.wTotalLength),
> > > > > > 					USB_DT_OTG, (void **) &desc) == 0) {
> > > > > > 			if (desc->bmAttributes & USB_OTG_HNP) {
> > > > > > 				unsigned		port1 = udev->portnum;
> > > > > > 
> > > > > > 				dev_info(&udev->dev,
> > > > > > 					"Dual-Role OTG device on %sHNP port\n",
> > > > > > 					(port1 == bus->otg_port)
> > > > > > 						? "" : "non-");
> > > > > > 
> > > > > > 				/* enable HNP before suspend, it's simpler */
> > > > > > 				if (port1 == bus->otg_port)
> > > > > > 					bus->b_hnp_enable = 1;
> > > > > > 				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,
> > > > > > 					0, NULL, 0, USB_CTRL_SET_TIMEOUT);
> > > > > > 
> > > > > > We're sending out this control request even if this host port is not OTG.
> > > > > > Isn't that wrong?
> > > > > 
> > > > > So send USB_DEVICE_A_ALT_HNP_SUPPORT request.
> > > > > This is correct in OTG 1.x, its intention is to remind the user who is
> > > > > connecting the HNP capable OTG device to a non-OTG port, He can change
> > > > > to another port of this machine which is a OTG port(with HNP).
> > > > 
> > > > I didn't understand.
> > > > If CONFIG_USB_OTG is enabled doesn't mean that this USB host port is OTG host.
> > > Yes, only CONFIG_USB_OTG enabled doesn't mean that this USB host port
> > > is a OTG port, there are might multiple usb host ports, but only one
> > > is a OTG port.
> > 
> > Not necessarily. Many systems don't have any OTG port so that request cannot
> > be sent even if CONFIG_USB_OTG is enabled.
> > 
> That's the fact, but we are talking the code only for those systems which
> have OTG port. If you think we need not cover the support for OTG 1.x host,
> we can simply remove it.

We should not remove it but need to add the following check.

Send that request only if the system has at least one OTG (v1.x) port that is
capable of HNP _and_ this port is not that HNP capable port _and_
the connected device is OTG (v1.x)

> 
> > > > So it should not send any OTG specific request to device. Right?
> > > Non-otg port(in OTG 1.x protocol) + OTG 1.x device, should send,
> > > otherwise, should not send.
> > 
> > What did you mean by Non-OTG port (in OTG 1.x protocol)?
> 
> Means host only port without HNP.
> 
> > If it is Non-OTG port it doesn't understand any OTG protocol.
> 
> With common sense, yes, but there is one exception.
> 
> > So Non-OTG port should not send any OTG request.
> > 
> see below from OTG 1.3:
> 
> 6.5.3 a_alt_hnp_support
> Setting this feature indicates to the B-device that it is connected to
> an A-device port that is not capable of HNP, but that the A-device does
> have an alternate port that is capable of HNP.
> The A-device is required to set this feature under the following conditions:
> • the A-device has multiple receptacles
> • the A-device port that connects to the B-device does not support HNP
> • the A-device has another port that does support HNP
> ... ...

Thanks for this info. I can't seem to find the OTG v1.3 spec.

cheers,
-roger

> 
> > > 
> > > So the code you pasted here was right only for OTG 1.x, I assume
> > > OTG 2.0 has not been released when it was designed. But now it's
> > > out of date, it's wrong if you connect a OTG 2.0 device. 
> > > 
> > 
> > The code is wrong for non-OTG ports when CONFIG_USB_OTG is set.
> > 
> > Peter/Felipe, any comments on this?
> > 
> > cheers,
> > -roger
> > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > 				if (err < 0) {
> > > > > > 					/* OTG MESSAGE: report errors here,
> > > > > > 					 * customize to match your product.
> > > > > > 					 */
> > > > > > 					dev_info(&udev->dev,
> > > > > > 						"can't set HNP mode: %d\n",
> > > > > > 						err);
> > > > > > 					bus->b_hnp_enable = 0;
> > > > > > 				}
> > > > > > 
> > > > > > Instead it should be moved inside the if (port1 == bus->otg_port) condition.
> > > > > 
> > > > > Nope, as I explained above, this is really too detailed OTG protocol:)
> > > > > > 
> > > > > > 			}
> > > > > > 		}
> > > > > > 	}
> > > > > > #endif
> > > > > > 	return err;
> > > > > > }
> > > > > > 
> > 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html