>
> Hi,
>
> On Fri, Aug 02, 2013 at 03:42:20PM +0000, Wang, Yu Y wrote:
> > > 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.
> >
> > [Yu:] Thanks for your help. Now I changed the configuration to add
> > quotation marks :)
>
> np
>
> > > > 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.
> >
> > [Yu:] Correct. So we have to re-initialize all relevant registers for
> > this requirement. And we want to maximize to re-use original
> > functions. But we find that we have to modify some code to achieve
> > this purposes.
>
> hmm.. weird. We already system suspend/resume working and I re-factored all
> code needed exactly to avoid duplication. Are you reading v3.11-rc3 code or
> some older tree ?
>
[Yu:] No. Haven't check latest code.
I will check 3.11 code for these new changes. Thanks.
> > > > > 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" ?
> >
> > [Yu:] The SW initialization is mean that no relevant hardware registers
> access.
> > The hardware settings is mean set some configurations via write
> > controller registers.
>
> alright, that's all factored out. If you read v3.11-rc3 there are two situations
> where we have memory allocation and register initialization together, but if you
> look into my 'next' branch in kernel.org, you'll see that I already have patches
> fixing those up.
[Yu:] Great. I will check your new patches for these changes.
>
> > > > > 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].
> >
> > [Yu:]This patch is good. But for hibernation mode, still need to set
> > these two PHY registers. Anyway, we can consider that to use other
> > implementations for them
>
> But hey, starting in version 1.94a of the core, the core itself will manage the
> PHY automatically. We shouldn't have to do anything with them. Besides, even
> if we _do_ have to manuall suspend the PHY when in hibernation, that's a
> power optimization thing, it shouldn't break anything if we leave the PHYs
> resumed ;-)
>
> Which means that this can be tackled later :-)
[Yu:] Correct. But with 2.10a RTL. There have some silicon BUGs. You can check the
section 1.9 of 2.10a spec. For some situation, we have to clear suspend enable
bit of GUSB2PHYCFG register. And for hibernation mode, we have to enable them
during suspend flow.
>
> > > > > 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.
> > >
> >
> > [Yu:] We are using 2.10a RTL now. But it will be upgrade to 2.50a in the near
> future.
> > I will verify the driver without do soft-reset every time. If it is
> > working, then we will try to follow your design. But I just concern the
> hibernation mode which is easy met
> > failures.
>
> Just do one favor to me before you try anything else:
>
> # mount -t debugfs none /sys/kernel/debug/ # modprobe dwc3-pci # modprobe
> dwc3 # modprobe xhci-hcd # grep DEPCMDPAR
> /sys/kernel/debug/dwc3*/regdump # dmesg | grep -i xhci
>
> Send me the output of those commands. Remember to enable xHCI debugging
> :-)
>
> I want to check if you guys have the register mirroring problem I described
> above.
[Yu:] Sure. I will help provide the registers dump after I return to office.
In home now. :-)
>
> > > > > 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.
> >
> > [Yu:] Sure, we are also do these test both for USB3 and USB3
> > components. And for hibernation Mode, we did lots of test and also
> > test with system power state(S0ix and Sx). Actually, our driver base
> > on K3.4 before. We are porting them to K3.10 now. And then consider to
> > design our codes with common architecture for upstream. After port done
> and stable with 2.50a RTL, we will prepare our patches to review by
> community.
>
> Sure, just try to port your patches the latest tag from Linus instead of
> v3.10 :-) You can also make sure you have the same changes in your internal
> tree and mainline so that there's zero backporting effort. I've done it for ages
> here in TI :-) git cherry-pick -sx helps a lot :-)
>
[Yu:] OK. We will. Thanks for your help.
> > Thanks a lot for your information. I learned lots of thing from you.
> > :)
>
> no problem, here to help.
>
> cheers
>
> --
> 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
