Re: [PATCH v11 08/14] usb: otg: add OTG/dual-role core

[Peter Chen]

On Wed, Jun 22, 2016 at 09:51:19AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <hzpeterchen@xxxxxxxxx> writes:
> >> >> >> > So, unless we use OTG FSM defined in OTG spec, we should not mention
> >> >> >> > "OTG" in Linux, right?
> >> >> >> 
> >> >> >> to avoid confusion with the terminology, yes. With that settled, let's
> >> >> >> figure out how you can deliver what your marketting guys are asking of
> >> >> >> you.
> >> >> >> 
> >> >> >
> >> >> > Since nxp SoC claims they are OTG compliance, we need to pass usb.org
> >> >> > test. The internal bsp has passed PET test, and formal compliance test
> >> >> > is on the way (should pass too). 
> >> >> >
> >> >> > The dual-role and OTG compliance use the same zImage, but different
> >> >> > dtb.
> >> >> 
> >> >> okay, that's good to know. Now, the question really is: considering we
> >> >> only have one user for this generic OTG FSM layer, do we really need to
> >> >> make it generic at all? I mean, just look at how invasive a change that
> >> >> is.
> >> >
> >> > If the chipidea is the only user for this roger's framework, I don't
> >> > think it is necessary. In fact, Roger introduces this framework, and
> >> > the first user is dwc3, we think it can be used for others. Let's
> >> 
> >> Right, we need to look at the history of dwc3 to figure out why the
> >> conclusion that dwc3 needs this was made.
> >> 
> >> Roger started working on this framework when Power on Reset section of
> >> databook had some details which weren't always clear and, for safety, we
> >> always had reset asserted for a really long time. It was so long (about
> >> 400 ms) that resetting dwc3 for each role swap was just too much.
> >> 
> >> Coupled with that, the OTG chapter wasn't very clear either on
> >> expections from Host and Peripheral side initialization in OTG/DRD
> >> systems.
> >> 
> >> More recent version of dwc3 databook have a much better description of
> >> how and which reset bits _must_ be asserted and which shouldn't be
> >> touched unless it's for debugging purposes. When I implemented that, our
> >> ->probe() went from 400ms down to about 50us.
> >> 
> >> Coupled with that, the OTG chapter also became a lot clearer to the
> >> point that it states you just don't initialize anything other than the
> >> OTG block, and just wait for OTG interrupt to do whatever it is you need
> >> to do.
> >> 
> >> This meant that we could actually afford to do full reinitialization of
> >> dwc3 on role swap (it's now only 50us anyway) and we knew how to swap
> >> roles properly.
> >> 
> >> (The reason for needing soft-reset during role swap is kinda long. But
> >> in summary dwc3 shadows register writes to both host and peripheral
> >> sides)
> >> 
> >> > just discuss if it is necessary for dual-role switch.
> >> 
> >> fair. However, if we have a single user we don't have a Generic
> >> layer. There's not enough variance to come up with truly generic
> >> architecture for this.
> >> 
> >> -- 
> >
> > I have put some points in my last reply [1], I summery it here to
> > see if a generic framework is deserved or not?
> >
> > 1. If there are some parts we can use during the role switch
> > - The common start/stop host and peripheral operation
> > eg, when switch from host to peripheral, all drivers can use
> > usb_remove_hcd to finish it.
> 
> a UDC such as dwc3 already implements start/stop for peripheral and
> host. Why would go through and indirection layer that just comes back to
> us? (well, dwc3's host side, start/stop translates to adding/removing
> xhci-plat's device)
> 
> > - A common workqueue to handle vbus and id event
> 
> I already have a threaded IRQ handler. Why do I need a workqueue?
> 

I know it can be done in individual driver, don't you think
we need a common part to manage the dual-role switch process,
since dual-role switch is used more and more common, and
there are so many switch methods:

- ID pin
- sysfs
- type-c
- OTG FSM
- Registers

Maybe Roger's framework is a little complicated, but if it is the
correct direction, we can improve it.
  
> > - sysfs for role switch
> 
> A generic sysfs is desirable, but I really don't know where to put it.
> Maybe it's enough to go down the hwmon route and just have an agreement
> of filename and contents to be written to.
> 
> -- 
> balbi



-- 

Best Regards,
Peter Chen