Re: [PATCH 0/3] Implement role-switch notifications from dwc3-drd to dwc3-qcom

[Peter Chen <peter.chen@kernel.org>]

On 21-07-04 02:33:11, Bryan O'Donoghue wrote:
> This is a topic we have been discussing for some time, initially in the
> context of gpio usb-c-connector role-switching.
> 
> https://lore.kernel.org/linux-usb/20200311191501.8165-1-bryan.odonoghue@linaro.org
> 
> Hardware availability constraints limited scope to finish that off.
> 
> Thankfully Wesley Cheng made a new set of USB role-switch related patches
> for dwc3-qcom, this time in conjunction with the qcom pm8150b type-c
> silicon.
> 
> https://lore.kernel.org/linux-usb/20201009082843.28503-1-wcheng@codeaurora.org
> 
> For the RB5 project we picked Wesley's changes and developed them further,
> around a type-c port manager.
> 
> As a precursor to that TCPM I reposted Wesley's patches
> https://lore.kernel.org/linux-usb/20210629144449.2550737-1-bryan.odonoghue@linaro.org
> 
> Bjorn pointed out that having the role-switch triggered from dwc3-qcom to
> dwc3-drd is not the right way around, indicating a preference for the
> original notifier from dwc3-drd to dwc3-qcom.
> 
> There are two approaches I considred and prototyped to accomplish the
> desired dwc3-drd -> dwc3-qcom messaging.
> 
> #1 Using a notifier in dwc3-drd to trigger dwc3-qcom
> 
>    This would be nice since it would accomplish the desired layering
>    dwc3-drd -> dwc3-qcom.
> 
>    However:
>    a) It would be a real mess as dwc3-qcom is the parent device of
>       dwc3-core so, if the child-device dwc3-core deferred probing for
>       whatever reason we would have to detect this and retry the parent's
>       probe again.

Why do you think we need to retry the parent's probe again? And why using
a notifier need to concern core's deferral probe? I know there are some
downstream code which using this way, I would like to know the shortcoming
for it.

Peter

>	The point in time that dwc3-qcom could potentially parse
>       such a deferral in the child device is late. It would also be weird
>       and messy to try to roll back the parent's probe because of a child
>       device deferral.
> 
>       I considered making some sort of worker in the parent to check for
>       child device probe but, again this seemed like an atrocious hack so,
>       I didn't even try to prototype that.
> 
>    b) One potential solution was using "__weak" linkage in a function
>       provided by dwc3-drd that a wrapper such as dwc3-qcom could then
>       over-ride.
> 
>       If a wrapper such as dwc3-qcom then implemented a function with
>       regular linkage it would over-ride the __weak function and provide a
>       method for the dwc3-drd code to call into dwc3-qcom when probing was
>       complete, thus allowing registration of the notifier when the child
>       was ready.
> 
>       This would work up until the point that you tried to compile two
>       implementations of a dwc3 wrapper into the one kernel module or the
>       one kernel image say dwc3-qcom and a similar implementation in
>       dwc3-meson. At that point you would get linker breakage.
> 
> #2 Using USB role switching for the notification
> 
>    Wesley's implementation took the approach dwc3-qcom -> dwc3-drd, whereas
>    the approach I'm proposing here is dwc3-drd -> dwc3-qcom, which is also
>    what we discussed on the list.
> 
>    Having implemented it, I think USB role-switching in the direction
>    dwc3-drd -> dwc3-qcom is also a much cleaner solution for several
>    reasons.
> 
>    a) Handling probe deferral is built into Linux' USB role switching today
>       so we don't have to re-invent that wheel, unlike with the original
>       notifier model.
> 
>    b) There is no "wiring up" or traversing the graph tree for the wrapper
>       layer to determine if the parent device has a compliant type-c
>       connector associated with it, unlike in the dwc3-qcom -> dwc3-drd
>       model.
> 
>       All that has to happen is "usb-role-switch" is declared in the parent
>       dwc3-qcom node and the role-switch API takes care of the rest.
> 
>       That means its possible to use a usb-c-connector, qcom type-c pm8150b
>       driver, a USCI, a tps659x, a fusb302 or something like ChromeOS
>       cros_ec to notify dwc3-drd without dwc3-qcom having to have
>       the slighest clue which type of device is sending the signal.
> 
>       All dwc3-qcom needs to do is waggle UTMI signals in a register when a
>       role-switch happens.
> 
>    c) It "feels" like a layering violation to have the dwc3-qcom SoC
>       wrapper receive the event and trigger the dwc3-drd core.
> 
>       The standard model of parent/child role switching or remote-endpoint
>       traversal that USB role switching already has works just fine for
>       dwc3-drd, we just need to trigger dwc3-qcom for the role-switch in a
>       non-vendor and non-SoC specific way.
> 
>    d) Less code. It turns out there's less code implementing as a
>       role-switch interface in the direction dwc3-drd -> dwc3-qcom.
> 
>    e) Portability. The mechanism used for dwc3-drd -> dwc3 qcom can be
>       reused for any other similar wrapper which models the wrapper as a
>       parent of the dwc3-drd.
> 
> For all of those reasons I've opted to use USB role-switch notification
> from dwc3-drd to dwc3-qcom.
> 
> git add bod git://git.linaro.org/people/bryan.odonoghue/kernel.git
> git fetch bod
> git diff usb-next-5.13.rcx-rb5-tcpm..usb-next-5.13.rcx-rb5-tcpm-v2
> 
> Bryan O'Donoghue (2):
>   usb: dwc3: Add role switch relay support
>   usb: dwc3: dwc3-qcom: Make dwc3-qcom a role-switch signal recipient
> 
> Wesley Cheng (1):
>   usb: dwc3: dwc3-qcom: Fix typo in the dwc3 vbus override API
> 
>  drivers/usb/dwc3/core.h      |  2 +
>  drivers/usb/dwc3/drd.c       | 10 +++++
>  drivers/usb/dwc3/dwc3-qcom.c | 77 ++++++++++++++++++++++++++++++++++--
>  3 files changed, 85 insertions(+), 4 deletions(-)
> 
> -- 
> 2.30.1
> 

-- 

Thanks,
Peter Chen