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@xxxxxxxxxx > > 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@xxxxxxxxxxxxxx > > 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@xxxxxxxxxx > > 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
