Hi,
On Mon, Jan 09, 2012 at 08:57:00AM -0800, Ido Shayevitz wrote:
> >> the core.c activates the gadget and host in case the DWC3_MODE is DRD.
> >> Instead I suggest that the core.c will activate the new otg.c driver
> >> which
> >> will implement the programming model as started in figure 12-4 in
> >> Synopsys
> >> spec.
> >> After the activation of the otg.c, it will check the
> >> GHWPARAMS6[10](=SRPSupport), for determining if OTG register space is
> >> available (as also illustrated in the figure 12-4) and if the otg
> >> interrupt handler can be settled, otg registers can be initialized,
> >> etc...
> >
> > Well, it would look something like:
> >
> > switch (mode) {
> > case DWC3_MODE_DRD:
> > ret = dwc3_otg_init(dwc);
> > /* error check */
> >
> > ret = dwc3_device_init(dwc);
> > /* error check */
> >
> > ret = dwc3_host_init(dwc);
> > /* error check */
> > break;
> > ...
> > }
> >
> >
> > dwc3_otg_init would look like:
> >
> > int dwc3_otg_init(struct dwc3 *dwc)
> > {
> > u32 reg;
> >
> > reg = dwc3_readl(dwc->regs, DWC3_GHWPARAMS6);
> >
> > if (!(reg & BIT(10)))
> > return -ENODEV;
> >
> > /* initialize OTG here */
> > }
> >
>
> Let's say that we have SRPSupport (means OTG_EN=1).
> Therefore wouldn't we like to activate the device or host according to the
> ID pin? I am saying that in the above code, the call to dwc3_otg_init will
that would be one way to do it and it would save us some memory and
somme register writes, but OTOH Host and Device events are independent
anyway and registering the gadget/host after ID pin changes might be too
late. On top of that, host and device events are mutually exclusive
anyway.
You would need to see how long does it take to register everything after
ID pin changes and see if you don't have any issues with any Hosts when
acting as device and any devices when acting as host.
> generate the calls to dwc3_device_init _OR_ dwc3_host_init in case that
> the SRPSupport is on and according to the ID pin. But later, returning to
> core probe, it will call again to the dwc3_device_init and dwc3_host_init.
> So one of the drivers will be initlized twice...
> Did I misunderstood you?
No no, dwc3_otg_init() should not call dwc3_device_init() or
dwc3_host_init(). dwc3_otg_init() will merely configure the OTG address
space, nothing more, nothing less :-)
> Maybe something like this:
>
> switch (mode) {
> case DWC3_MODE_DRD:
> ret = dwc3_otg_init(dwc);
> /* error check */
> break;
> case DWC3_MODE_DEVICE:
> ret = dwc3_device_init(dwc);
> /* error check */
> break;
> case DWC3_MODE_HOST:
> ret = dwc3_host_init(dwc);
> /* error check */
> break;
> ...
> }
>
>
> dwc3_otg_init would look like:
>
> int dwc3_otg_init(struct dwc3 *dwc)
> {
> u32 reg;
>
> reg = dwc3_readl(dwc->regs, DWC3_GHWPARAMS6);
>
> if (!(reg & BIT(10))) {
> /* No OTG support, just activating device+host drivers*/
> ret = dwc3_device_init(dwc);
> /* error check */
>
> ret = dwc3_host_init(dwc);
> /* error check */
>
> return -ENODEV;
> }
I don't like this because it makes a piece of code which was supposed to
handle OTG alone, poke into Host and Device. I don't want that, really.
Keeping the xHCI bus always available and the UDC always available is
likely not to cause any problems for you.
> >> As for the phy logic, I am not sure where it's goes in this design, I
> >> tried to follow the "OTG rework" loop and I understand that this in
> >> check
> >> by Kishon?
> >
> > Yes, we will have something to show soonish, right now we are really,
> > really busy with some internal stuff.
> >
>
> OK, I just wanted to mention that I am not sure about this.
fair enough ;-)
> >> Please correct or comment me on the above.
> >> If it is ok with you, I think I will be able to supply some initial
> >> patch to review in a few days.
> >
> > That's ok, but before sending anything be sure to run checkpatch.pl and
> > sparse on each and every single patch. Also make sure to compile on your
> > architecture and on x86. Do *NOT* introduce any compile warnings/errors
> > to that driver and make sure to follow CodingStyle correctly. Take a
> > look at SubmittingPatches and SubmitChecklist and I need to know how you
> > tested the patches. It should be ok to add a note about testing on your
> > series' cover letter.
> >
> > (for ease compile testing:
> >
> > $ make allmodconfig
> > $ make -j8 drivers/usb > /dev/null
> >
> > $ make ARCH=arm my_device_defconfig
> > $ make ARCH=arm menuconfig
> > (enable everything USB as modules and I really mean everything)
> > $ make ARCH=arm =j8 drivers/usb > /dev/null)
> >
> > If these aren't followed, I will go into rant mode hehe ;-)
> >
>
> Cool, no problem, I will ask you if I get trouble with something.
that would be great :-) Thanks
--
balbi
Attachment:
signature.asc
Description: Digital signature
