On 22/04/15 12:22, Peter Chen wrote: > On Wed, Apr 22, 2015 at 10:33:24AM +0300, Roger Quadros wrote: >> On 22/04/15 05:17, Peter Chen wrote: >>> On Tue, Apr 21, 2015 at 10:34:01AM +0300, Roger Quadros wrote: >>>> On 21/04/15 09:04, Peter Chen wrote: >>>>> >>>>>> >>>>>> On 20/04/15 06:05, Peter Chen wrote: >>>>>>> On Tue, Apr 14, 2015 at 01:41:47PM +0300, Roger Quadros wrote: >>>>>>>> This is an attempt to centralize OTG/Dual-role functionality in the kernel. >>>>>>>> As of now I've got Dual-role functionality working pretty reliably on >>>>>>>> dra7-evm. xhci side of things for OTG/DRD use are fixed in >>>>>>>> http://thread.gmane.org/gmane.linux.kernel/1923161 >>>>>>> >>>>>>> Hi Roger, >>>>>>> >>>>>>> Currently, there are two main problems for DRD/OTG framework. >>>>>>> >>>>>>> - For multi-platform supports, we may define CONFIG_USB_OTG, but the >>>>>>> gadget should not add its otg descriptor to its configuration >>>>>>> descriptors if it does not support one of otg features (SRP/HNP/ADP). >>>>>>> Macpaul Lin's patch set [1] is the right way to do it. >>>>>> >>>>>> Agreed. That check (whether OTG descriptor can be added and which version >>>>>> of it) has to be done at runtime and it must be added only if hardware supports >>>>>> OTG _and_ kernel OTG support is enabled. >>>>>> >>>>> >>>>> Ok, let's put this patch set in mainline first, since your patch set may need some >>>>> information from it. >>>>> >>>>>>> - We are lack of framework to handle OTG (DRD) switch, it is great you >>>>>>> are designing it. The main problem for this framework is how to handle >>>>>>> DRD/OTG FSM unify. My thought is we add two paths for them separate. >>>>>>> For easy, I suggest if the platform supports one of otg features, then >>>>>>> it goes to fully otg fsm, else it goes to simply otg fsm (like your >>>>>>> drd fsm). If you agree with it too, you may not need to add another >>>>>> "dr_mode" >>>>>>> value. >>>>>> >>>>>> It would be nice that way but unfortunately it does't work in all cases. >>>>>> e.g. What if the SoC itself supports all OTG features but the board is not >>>>>> designed for OTG. Or the product designer simply is not interested in full OTG >>>>>> support but just dual-role. So we need some flexibility for the device >>>>>> tree/platform-data to specify that. This is where a new "dr_mode" == "dual- >>>>>> role" is needed. >>>>>> >>>>> >>>>> Since "dr_mode" has been widely used now, if we add a new property for it, we >>>>> need to change all drivers. >>>>> >>>>> Your OTG/DRD framework needs to (partial) use otg fsm, and we will not teach old >>>>> driver to use it since there are some driver related stuffs. >>>> >>>> fair enough. Let's not change dr_mode then and decide based on other parameters. >>>> >>>>> >>>>> SRP/HNP/ADP support can be board level capabilities, and we may consider the >>>>> otg device which does not support otg fsm (hardware finishes fsm). So I suggest >>>>> we have below properties at dts: >>>>> >>>>> - otg-support /* fully otg support */ >>>>> - otg-fsm-support /* fully otg fsm support */ >>>> >>>> what is the difference between otg-support and otg-fsm-support? >>> >>> Like I mentioned at above, the hardware finishes HNP/SRP which does not >>> use otg fsm code (usb-otg-fsm.c), most of legacy otg platforms (musb?) >>> use this way, for these platforms, only need to set otg-support = 1 >> >> So dr_mode = "otg" _and_ otg-support = 1? >> > > Yes > >> Again wouldn't this involve changes to dts for musb like platforms >> supporting full OTG? >> >> Instead we could add a new field saying otg-fsm-type >> "otg-hw", "otg-sw", "drd-sw" >> >> If the field is absent it defaults to "otg-hw". >> This also means we don't need otg-fsm-support flag. >> > > Yes, that's we doesn't need to change for old platforms, but > we keep our default value as small possibilities, most of current > platforms are only drd platform, it is hard choice. > > My original thought was nothing need to add at dts for drd device. > >> Now the pseudo-code to decide fsm is >> >> if (dr_mode == "otg" && CONFIG_USB_OTG) >> if (otg-fsm-type == "otg-sw") { >> if (CONFIG_USB_OTG_FSM) >> full otg fsm support via sw >> else >> error "CONFIG_USB_OTG_FSM" not set >> } else if (otg-fsm-type == "drd-sw") { >> dual role fsm support >> } else { >> full otg support via hw >> } >> >> if (otg-fsm-type == "otg >> else >> error "CONFIG_USB_OTG" not set > > So we will have a separate drd fsm file, and the CONFIG_USB_OTG > and CONFIG_USB_OTG_FSM are not needed to be defined, right? > for drd case CONFIG_USB_OTG_FSM is definitely not needed. I'm not sure if we can operate dual-role without CONFIG_USB_OTG. I was thinking of combining the OTG core functionality (drivers/usb/common/usb-otg.c) with CONFIG_USB_OTG. >> >>> >>> For platforms which software finishes HNP/SRP using otg fsm code, need >>> to set both flags. >>> >>> For platforms which only do role switch through id pin, do not need to >>> set both. >>> >> >> OK. I get it now. >> >>>> >>>>> - otg-ver /* eh & otg supplement version */ >>>> >>>> we can get otg version from the OTG controller. What exactly is the >>>> otg-ver in dts for? >>> >>> Since most of otg stuffs are software's, eg, for otg descriptor, we will >>> only use otg 2.0 descriptor when both CONFIG_USB_OTG20 and otg-ver = 20 >>> are set. >> >> CONFIG_USB_OTG20 is redundant now. Plus I mentioned in the respective thread >> that it is not suitable for booting single image on different platforms. >> > > Ok, agree. Then we need to define two usb otg descriptors > for both 1.3 and 2.0, and we need to have two otg descriptor array > at gadget driver. And the code at gadget driver will like below: > > static const struct usb_descriptor_header *otg_desc_13[] = { > (struct usb_descriptor_header *) &(struct usb_otg_descriptor_13){ > .bLength = sizeof(struct usb_otg_descriptor_13), > .bDescriptorType = USB_DT_OTG, > > /* > * REVISIT SRP-only hardware is possible, although > * it would not be called "OTG" ... > */ > .bmAttributes = USB_OTG_SRP | USB_OTG_HNP, > }, > NULL, > }; > > static const struct usb_descriptor_header *otg_desc_20[] = { > (struct usb_descriptor_header *) &(struct usb_otg_descriptor_20){ > .bLength = sizeof(struct usb_otg_descriptor_20), > .bDescriptorType = USB_DT_OTG, > > /* > * REVISIT SRP-only hardware is possible, although > * it would not be called "OTG" ... > */ > .bmAttributes = USB_OTG_SRP | USB_OTG_HNP, > .bcdOTG = cpu_to_le16(0x0200), > }, > NULL, > }; instead of hardcoding bmAttributes field, it must be obtained from the appropriate data structure that was set by the controller driver by reading DT and OTG hardware info. > > During bind process: > > if (gadget_is_otg_13(c->cdev->gadget)) > c->descriptors = otg_desc_13; > else if (gadget_is_otg_20(c->cdev->gadget)) > c->descriptors = otg_desc_20; Probably a helper utitily can do the necessary checks and build the otg descriptor for us. usb_otg_get_descriptor(c->descriptors); e.g. for OTG3.0 we have a new flag USB_OTG_RSP, and this helper can be upgraded in the future. > >> As of now I can see 2 inputs regarding otg-ver >> - One comes from otg-ver DT property or platform data. >> - Second that may come from OTG controller registers. e.g. It might support >> OTG v3.0 but system designer wants to limit to OTG v2.0 via otg-ver. >> >> Controller driver can decide among the 2 and set the appropriate otg version >> in the data structure. >> > > Yes, it is the same we do for usb mode. OK. cheers, -roger > >>> >>>> >>>>> - adp-support /* board adp support */ >>>>> - srp-support /* board srp support */ >>>>> - hnp-support /* board hnp support */ >>>> >>>> So if these options are not provided in DTS but the OTG core supports them then >>>> we keep the respective feature disabled? >>> >>> Yes. >>> >>>> Won't this need dts change for existing boards? >>> >>> Does you know any dts based platform supports hnp/srp? >> >> I'm the wrong person to ask this. Maybe Felipe/Tony can comment. >> Irrespective of whether any dts platforms supports hsn/srp or not >> we must assume that till date dr_mode = otg implies full otg support >> and we cannot change its meaning. >> >> We can add new fields to indicate dual-role mode which is a new feature. > >> >>> For chipidea platform, currently, we depends on kernel >>> configurations (CONFIG_USB_OTG_FSM), but it is incorrect way. >>> >>>> >>>> Instead how about having disable flags instead. >>>> - adp-disable /* board doesn't support adp */ >>>> - srp-disable /* board doesn't support srp */ >>>> - hnp-disable /* board doesn't support hnp */ >>>> > > Like I mentioned above, it depends on which otg features we would like > to give as default features for dual-role device. > > For switching the role through the ID pin devices, we doesn't need any otg > features. > For adp/srp/hnp supported devices, we need (partial) otg features, and > the fsm (hardware or software) are needed. > >>>> Now, if the flags are not provided in dts we use the OTG core's flags. >>>> >>> >>> How the OTG core's know if it supports these? >> >> By OTG core, I meant OTG controller core. At least the dwc3 OTG controller has >> register bits to identify if it supports adp/srp/hnp. >> >> But we also need to keep in mind that adp feature can be provided separately even >> if the OTG controller core doesn't support it. >> >> cheers, >> -roger >> >>> >>>>> >>>>> Currently, if CONFIG_USB_OTG and CONFIG_USB_OTG_FSM are enabled, we will >>>>> have otg fsm code (usb-otg-fsm.c). >>>>> >>>>> if (otg-support & otg-fsm-support) >>>>> this device has fully otg support, and will follow full otg fsm transitions. >>>>> else >>>>> this device is drd, and will follow simple otg fsm transtions. >>>>> >>>> >>>> cheers, >>>> -roger >>>> >>> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
