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

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4386 bytes --]


Hi,

Peter Chen <hzpeterchen@gmail.com> 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,

A common part will be a requirement when we have at least 3 users for
it. Right now there's only one. So how can this be common at all?

> 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.

IMO, we don't have enough users

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]