Hi Balibi, Check my comments as follows. Thanks, Regards, Yu -----Original Message----- From: Felipe Balbi [mailto:balbi@xxxxxx] Sent: Friday, August 02, 2013 6:34 PM To: Wang, Yu Y Cc: balbi@xxxxxx; Li, Jiebing; Linux USB Mailing List; Zhuang, Jin Can; Wu, Hao; Yuan, Hang Subject: Re: About the hibernation feature implementation in dwc3 driver Hi, no top-posting please, limit your lines to 80-characters On Fri, Aug 02, 2013 at 09:25:39AM +0000, Wang, Yu Y wrote: > Thanks the quick response. > Actually, our solution is familiar with your thinking. > > Due to we have to do "Power-On or Soft Reset" for disconnect and > re-connect case, so driver need re-initialized all hardware registers. you don't need a full Soft-reset when disconnecting the cable. Read the driver and you'll we don't soft-reset the IP at every disconnect. [Yu] I don't know what version of your spec. My spec version is "Version 2.10a January 2012", in section 12.2.3.4. " Configure the core as described in "Device Power-On or Soft Reset". " Spec require the core do soft-reset. > That's why we want to separate register initialization operation from > software part (like sw structure init and endpoint initialization) register initialization is also SW, so I don't know what you want to split here. [Yu] We just want to split the SW initialization and hardware settings. > However, we find that the hardware registers initialization is located > in dwc3_gadget_init and 'dwc->gadget_driver' initialization is handled > in dwc3_gadget_start.() So we can't full re-use these two functions. this is wrong. dwc3_gadget_init() does nothing more than allocate memory. [Yu] In kernel3.10. In dwc3_gadget_init(), We find some hardware setting. For example, set clear suspend enable bit of GUSB2PHYCFG and GUSB3PIPECFG registers. Maybe we need to check the new version? > If all the hardware initialization can be extract into a new function > which call by udc_start callback and also can be called for > hibernation disconnect case, then it is fine. that's already how it works. Just read the code. > And for disconnect, our driver need to do some hardware clear work. > For example, disable irq, clear events which haven't handled, and > disable endpoints and so on. that's already how it works. Just read the code. > BTW: Besides that. In our design, we also support host mode, and also > have one OTG driver to maintain the role switch and USB charger > detection. right, Ido from codeaurora had some design to support but the work was lost since he stopped sending newer versions. > For role switch, our implementation is not only set GCTL.PORTCAP. Also > we implemented some API for OTG driver, for example: > start_host/stop_host and start_peripheral/stop_peripheral. alright, now go ahead and remove those since they're likely not needed. > All device and host hardware initialization will be done in these API. > So for connect/disconnect case, the driver will re-initialization the > host or device stack. this wastes too much time and is usually unnecessary. [Yu] Base on this design, the core will be reset during role switching. Not only controller, also for the PHY. We will try our design to confirm if working. > I saw your design is do all hardware initialization during kernel > boot. And just use GCTL.PORTCAP to do the role switch. I haven't try > this solution on intel platform. And not sure if is working with > hibernation feature. right, then test and let me know the results. Also, instead of spending so much time telling me what kind of changes Intel has in their own hidden tree, why don't you go ahead and send patches for review ? That would be so much easier for both of us... [Yu] Actually. The hibernation feature waste us lots of time. And we will try your design to confirm if hibernation mode still working. And we will also try to change our drivers to better upstream. After that, We will provide the patch to review. -- balbi -- 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
