Hi,
On Fri, Aug 02, 2013 at 10:54:18AM +0000, Wang, Yu Y wrote:
> Check my comments as follows.
weird, you sent plain text email, but there are no quotation marks... it
makes it very difficult to read. Please go through our netiquette, there
is a copy of that at [1] and also lots of good information under
Documentation, including tips on how to configure many email client.
> 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.
As described in that, but it doesn't mean we need to do a full SW reset
of the IP. It just means we need to setup DCTL and all those other
registers again.
> > 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.
and what do you mean by that ? What are you calling "SW initialization"
and what do you consider to be "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?
hmm, that code wouldn't even run in version 2.10a of the core, it's for
versions before 1.94a as you can see below:
| int dwc3_gadget_init(struct dwc3 *dwc)
| {
| u32 reg;
| int ret;
[ ... ]
| /* Enable USB2 LPM and automatic phy suspend only on recent versions */
| if (dwc->revision >= DWC3_REVISION_194A) {
| dwc3_gadget_usb2_phy_suspend(dwc, false);
| dwc3_gadget_usb3_phy_suspend(dwc, false);
| }
[ ... ]
| }
and, recently, I've dropped support for manual PHY control, see [2].
> > 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.
that's way too much time wasted. You *really* don't need to reset the
whole thing just to change roles. I doubt that's how Synopsys designed
the IP, if they did then nobody will ever pass the tight timing
requirements imposed by OTG specification.
The only problem I know about in that area is that the IP mirrors some
device registers to important xHCI register. We have a case with
Synopsys to track that but I'd say all versions suffer from that bug all
the way to 2.50a, IIRC 2.60a has that sorted out.
BTW, it might very well be that because of this problem you're
suggesting that we Soft-Reset the IP, perhaps Synopsys didn't give you
the full gory details ? But in summary, writing to some of the DEPCMDPAR
registers, mirrors the same value into the xHCI's Ring registers, thus
destroying any functionality when switching from device to host and
back.
We need to *really* think how we want to support these older versions of
the Synopsys IP since that will be a *really* invasive change on both
dwc3 and xhci drivers. My *strong* suggestion is to focus on stabilizing
role switching before trying to implement hibernation.
> > 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.
sure, that'd be great. Patches are always welcome. Just make sure you
properly test hibernation and make sure it doesn't break anything. As of
today we have mainline Linux + mainline dwc3 passing the entire USB3
Peripheral side certification, all tests including Link Layer, USB30CV,
interop and whatnot, so I'm very reluctant to let any features in which
are not properly tested.
cheers
[1] http://elinux.org/Netiquette
[2] http://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=next&id=636e2a2caeafcb1b55b6799b29fe436039819eb9
--
balbi
Attachment:
signature.asc
Description: Digital signature
