| Message ID | 20210704013314.200951-1-bryan.odonoghue@linaro.org |
|---|---|
| Headers | show |
| Series | Implement role-switch notifications from dwc3-drd to dwc3-qcom | expand |
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 >
On 21-07-07 14:03:19, Bjorn Andersson wrote: > On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote: > > Allow me to reorder your two questions: > > > And why using a notifier need to concern core's deferral probe? > > The problem at hand calls for the core for somehow invoking > dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed. > > This means that dwc3-qcom somehow needs to inform the dwc3-core about > this (and stash the pointer). And this can't be done until dwc3-core > actually exist, which it won't until dwc3_probe() has completed > successfully (or in particular allocated struct dwc). Maybe you misunderstood the notifier I meant previous, my pointer was calling glue layer API directly. Role switch is from dwc3-core, when it occurs, it means structure dwc3 has allocated successfully, you could call glue layer notifier at function dwc3_usb_role_switch_set directly. Some references of my idea [1] [2] [1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event [2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205 > > > Why do you think we need to retry the parent's probe again? > > There's four options here: > > 0) Hope that of_platform_populate() always succeeds in invoking > dwc3_probe() on the first attempt, so that it is available when > of_find_device_by_node() is invoked in dwc3_qcom_probe() (and a few of > the other platform's drivers). > > 1) Ensure that the operations performed by dwc3_probe() happens > synchronously and return a failure to dwc3-qcom, which depending on how > dwc3_probe() failed can propagate that failure - i.e. either probe defer > or clean up its resources if the failure from dwc3-core is permanent. > > 2) Register the dwc3-core and then return from dwc3_qcom_probe() in some > half-initialized state and through some yet to be invented notification > mechanism continue the tail of dwc3_qcom_probe() once dwc3_probe() has > finished. But I know of no such notification mechanism in place today > and we can just register a callback, because of 1). > Furthermore we'd leave dwc3-qcom half-initialized until the dwc3-core is > done probing - which might never happen. > > 3) Make drvdata of all the platform drivers be some known struct that > dwc3-core can retrieve and dereference - containing the optional > callback to the role_switch callback. > > > We've tried the option 0) for a few years now. Option 2) is a variant of > what we have today, where we patch one problem up and hope that nothing > unexpected happens until things has fully probed. We're doing 3) in > various other places, but in my view it's abusing a void * and has to be > kept synchronized between all the possible parent drivers. > > Left is 1), which will take some refactoring, but will leave the > interaction between the two drivers in a state that's very easy to > reason about. Function of_find_device_by_node() invoked at glue layer is usually successfully, The dwc3_probe failure doesn't affect it, unless you enable auto-suspend, and glue layer's runtime suspend routine depends on dwc3 core's runtime suspend routine. Would you please describe more about dwc3-core probe failure causes dwc3-qcom's probe has failed or in half-initialized state you said? > > > I know there are some downstream code which using this way, I would > > like to know the shortcoming for it. > > > > The shortcoming of having dwc3_qcom_probe() invoke dwc3_probe() > "manually" and then returning -EPROBE_DEFER if the dwc3-core's resources > aren't yet available is that we're wasting some time tearing down the > dwc3-qcom state and then re-initialize it next time this is attempted. Like above, would you explain more about it? -- Thanks, Peter Chen
On 08/07/2021 04:54, Bjorn Andersson wrote: > Bryan had a previous patch where the glue layer was notified about role > switching (iirc) and as soon as we hit a probe deferal in the core > driver we'd dereference some pointer in the glue layer. I don't find the > patch right now, but I suspect it might have been caused by the same > platform_get_drvdata() as we see in qcom_dwc3_resume_irq(). Here https://lore.kernel.org/linux-usb/20200311191501.8165-7-bryan.odonoghue@linaro.org/ and here https://lore.kernel.org/linux-usb/20200311191501.8165-8-bryan.odonoghue@linaro.org/ one thing about that I don't think is right now in retrospect is having to find a DT connector in the core, meaning it incorrectly assumes you have a node named "connector" as a child of dwc3-core https://lore.kernel.org/linux-usb/158463604559.152100.9219030962819234620@swboyd.mtv.corp.google.com/ Having done some work with TCPM on pm8150b silicon I see what Stephen was saying about that That's one solid reason I like the USB role-switch API - because it gets you out of the business of trying to discern from dwc3-qcom if dwc3-core has role-switching turned on by iterating through its range of DT nodes and looking for a special one named "connector" The initial and imperfect solution I had for that looked like this if (dwc3_qcom_find_gpio_usb_connector(qcom->dwc3)) {} Wesley had another iteration on that that was a little better https://lore.kernel.org/linux-usb/20201009082843.28503-4-wcheng@codeaurora.org/ +static int dwc3_qcom_connector_check(struct fwnode_handle *fwnode) +{ + if (fwnode && (!fwnode_property_match_string(fwnode, "compatible", + "gpio-usb-b-connector") || + !fwnode_property_match_string(fwnode, "compatible", + "usb-c-connector"))) + return 1; + + return 0; +} All we are really doing there is replicating functionality that the role-switch API already provides --- bod
On Wed 07 Jul 20:06 PDT 2021, Peter Chen wrote: > On 21-07-07 14:03:19, Bjorn Andersson wrote: > > On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote: > > > > Allow me to reorder your two questions: > > > > > And why using a notifier need to concern core's deferral probe? > > > > The problem at hand calls for the core for somehow invoking > > dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed. > > > > This means that dwc3-qcom somehow needs to inform the dwc3-core about > > this (and stash the pointer). And this can't be done until dwc3-core > > actually exist, which it won't until dwc3_probe() has completed > > successfully (or in particular allocated struct dwc). > > Maybe you misunderstood the notifier I meant previous, my pointer was > calling glue layer API directly. > > Role switch is from dwc3-core, when it occurs, it means structure dwc3 has > allocated successfully, you could call glue layer notifier at function > dwc3_usb_role_switch_set directly. > Some references of my idea [1] [2] > > [1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event > [2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205 > Hi Peter, I took a proper look at this again, hoping to find a way to pass a callback pointer from dwc3-qcom to the dwc3 core, that can be called from __dwc3_set_mode() to inform the Qualcomm glue about mode changes. This looks quite promising and I think we should be able to get rid of some duplicated extcon code from the Qualcomm glue as well... I reworked the existing code to replace of_platform_populate() with of_platform_device_create_pdata() to register the core device and pass a struct with a function pointer and this works fine. But then I searched the list archives and found this: https://lore.kernel.org/lkml/CAL_JsqJ5gsctd7L3VOhTO1JdUqmMmSJRpos1XQyfxzmGO7wauw@mail.gmail.com/ In other words, per Rob's NACK this seems like a dead end. @Rob, for your understanding, the dwc3 platform glue is implemented in a set of platform_drivers, registering the core as a separate platform_device and we need to make a function call from the core dwc3 code into the glue code. My initial proposal was that we provide some helper function that the glue code would use to allocate and register the core device, along which we can pass such callback information. But this turns out to pretty much be a re-implementation of of_platform_device_create_pdata(). Perhaps that's still preferable? Regards, Bjorn
On Thu 08 Jul 03:17 PDT 2021, Bryan O'Donoghue wrote: > On 08/07/2021 04:54, Bjorn Andersson wrote: > > Bryan had a previous patch where the glue layer was notified about role > > switching (iirc) and as soon as we hit a probe deferal in the core > > driver we'd dereference some pointer in the glue layer. I don't find the > > patch right now, but I suspect it might have been caused by the same > > platform_get_drvdata() as we see in qcom_dwc3_resume_irq(). > > Here > > https://lore.kernel.org/linux-usb/20200311191501.8165-7-bryan.odonoghue@linaro.org/ > > and here > > https://lore.kernel.org/linux-usb/20200311191501.8165-8-bryan.odonoghue@linaro.org/ > Now that I dug through the code I remembered why this didn't work. You do: dwc = platform_get_drvdata(qcom->dwc3); In order to be able to register the callback in the notifier chain that you added to struct dwc3, but while qcom->dwc3 is a valid struct platform_device, it might not have probed yet and "dwc" becomes NULL, which you then dereferenced in dwc3_role_switch_notifier_register(). So we need a mechanism that passes that callback/notifier that has a life cycle matching that of the glue device. > one thing about that I don't think is right now in retrospect is having to > find a DT connector in the core, meaning it incorrectly assumes you have a > node named "connector" as a child of dwc3-core > > https://lore.kernel.org/linux-usb/158463604559.152100.9219030962819234620@swboyd.mtv.corp.google.com/ > > Having done some work with TCPM on pm8150b silicon I see what Stephen was > saying about that > > That's one solid reason I like the USB role-switch API - because it gets you > out of the business of trying to discern from dwc3-qcom if dwc3-core has > role-switching turned on by iterating through its range of DT nodes and > looking for a special one named "connector" > > The initial and imperfect solution I had for that looked like this > > if (dwc3_qcom_find_gpio_usb_connector(qcom->dwc3)) {} > > Wesley had another iteration on that that was a little better > > https://lore.kernel.org/linux-usb/20201009082843.28503-4-wcheng@codeaurora.org/ > > +static int dwc3_qcom_connector_check(struct fwnode_handle *fwnode) > +{ > + if (fwnode && (!fwnode_property_match_string(fwnode, "compatible", > + "gpio-usb-b-connector") || > + !fwnode_property_match_string(fwnode, "compatible", > + "usb-c-connector"))) > + return 1; > + > + return 0; > +} > > All we are really doing there is replicating functionality that the > role-switch API already provides > But isn't this role switching interaction (both B and C) already part of the core/drd, so if we can just get the drd to invoke dwc3_qcom_vbus_overrride_enable() we're done (and can remove all the extcon code from the qcom glue as well)? Regards, Bjorn
On 25/08/2021 00:52, Bjorn Andersson wrote: > But isn't this role switching interaction (both B and C) already part of > the core/drd, so if we can just get the drd to invoke > dwc3_qcom_vbus_overrride_enable() we're done (and can remove all the > extcon code from the qcom glue as well)? Provided we have an acceptable way of triggering when a role-switch happens - then USB role switching itself is a NOP here, its really just a convenience to invoke the callback. --- bod
On Tue 24 Aug 16:58 PDT 2021, Bryan O'Donoghue wrote: > On 25/08/2021 00:52, Bjorn Andersson wrote: > > But isn't this role switching interaction (both B and C) already part of > > the core/drd, so if we can just get the drd to invoke > > dwc3_qcom_vbus_overrride_enable() we're done (and can remove all the > > extcon code from the qcom glue as well)? > > Provided we have an acceptable way of triggering when a role-switch happens > - then USB role switching itself is a NOP here, its really just a > convenience to invoke the callback. > Thanks for confirming. Then let's come up with an acceptable way, rather than duplicating yet another feature already implemented in the core. Regards, Bjorn
On 25/08/2021 01:01, Bjorn Andersson wrote: > On Tue 24 Aug 16:58 PDT 2021, Bryan O'Donoghue wrote: > >> On 25/08/2021 00:52, Bjorn Andersson wrote: >>> But isn't this role switching interaction (both B and C) already part of >>> the core/drd, so if we can just get the drd to invoke >>> dwc3_qcom_vbus_overrride_enable() we're done (and can remove all the >>> extcon code from the qcom glue as well)? >> >> Provided we have an acceptable way of triggering when a role-switch happens >> - then USB role switching itself is a NOP here, its really just a >> convenience to invoke the callback. >> > > Thanks for confirming. Then let's come up with an acceptable way, rather > than duplicating yet another feature already implemented in the core. > > Regards, > Bjorn > The only other thing to say about USB role-switching is it appears to be very much 1:1. Extcon allows a virtually unlimited number of consumers of an even. Is it envisaged that a role-switch could or should be consumed by say 3 or even 4 separate pieces of logic and if not, why not ? What if we had a magic black box that needed to sit ontop of the QCOM layer and further consume a role switch ? notifier/platform pointer + callback ? --- bod
On Wed, Aug 25, 2021 at 12:52 AM Felipe Balbi <balbi@kernel.org> wrote: > > > Hi, > > Bjorn Andersson <bjorn.andersson@linaro.org> writes: > > On Wed 07 Jul 20:06 PDT 2021, Peter Chen wrote: > > > >> On 21-07-07 14:03:19, Bjorn Andersson wrote: > >> > On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote: > >> > > >> > Allow me to reorder your two questions: > >> > > >> > > And why using a notifier need to concern core's deferral probe? > >> > > >> > The problem at hand calls for the core for somehow invoking > >> > dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed. > >> > > >> > This means that dwc3-qcom somehow needs to inform the dwc3-core about > >> > this (and stash the pointer). And this can't be done until dwc3-core > >> > actually exist, which it won't until dwc3_probe() has completed > >> > successfully (or in particular allocated struct dwc). > >> > >> Maybe you misunderstood the notifier I meant previous, my pointer was > >> calling glue layer API directly. > >> > >> Role switch is from dwc3-core, when it occurs, it means structure dwc3 has > >> allocated successfully, you could call glue layer notifier at function > >> dwc3_usb_role_switch_set directly. > >> Some references of my idea [1] [2] > >> > >> [1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event > >> [2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205 > >> > > > > Hi Peter, I took a proper look at this again, hoping to find a way to > > pass a callback pointer from dwc3-qcom to the dwc3 core, that can be > > called from __dwc3_set_mode() to inform the Qualcomm glue about mode > > changes. > > I would rather keep the strict separation between glue and core. On the surface that seems nice, but obviously there are issues with the approach. It's also not how pretty much every other instance of licensed IP blocks are structured. The specific need here seems to be multiple entities needing role switch notifications. Seems like that should be solved in a generic way. Rob
Hi, Bjorn Andersson <bjorn.andersson@linaro.org> writes: >> Bjorn Andersson <bjorn.andersson@linaro.org> writes: >> > On Wed 07 Jul 20:06 PDT 2021, Peter Chen wrote: >> > >> >> On 21-07-07 14:03:19, Bjorn Andersson wrote: >> >> > On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote: >> >> > >> >> > Allow me to reorder your two questions: >> >> > >> >> > > And why using a notifier need to concern core's deferral probe? >> >> > >> >> > The problem at hand calls for the core for somehow invoking >> >> > dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed. >> >> > >> >> > This means that dwc3-qcom somehow needs to inform the dwc3-core about >> >> > this (and stash the pointer). And this can't be done until dwc3-core >> >> > actually exist, which it won't until dwc3_probe() has completed >> >> > successfully (or in particular allocated struct dwc). >> >> >> >> Maybe you misunderstood the notifier I meant previous, my pointer was >> >> calling glue layer API directly. >> >> >> >> Role switch is from dwc3-core, when it occurs, it means structure dwc3 has >> >> allocated successfully, you could call glue layer notifier at function >> >> dwc3_usb_role_switch_set directly. >> >> Some references of my idea [1] [2] >> >> >> >> [1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event >> >> [2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205 >> >> >> > >> > Hi Peter, I took a proper look at this again, hoping to find a way to >> > pass a callback pointer from dwc3-qcom to the dwc3 core, that can be >> > called from __dwc3_set_mode() to inform the Qualcomm glue about mode >> > changes. >> >> I would rather keep the strict separation between glue and core. >> > > I'm okay with that goal, but the result is that both the OMAP and > Qualcomm driver duplicates the extcon interface already present in the > DRD, and the Meson driver duplicates the usb_role_switch. In addition to > the code duplication this manifest itself in the need for us to link > extcon to both the glue and core nodes in DeviceTree. > > In order to function in a USB-C based setup we now need to register a > usb_role_switch from the Qualcomm glue and we need to evolve the > usb_role_switch implementation to allow for the Type-C controller to > notify more than a single role-switcher. > > So we're facing the need to introduce another bunch of duplication and > the DT will be quite ugly with both glue and core having to set up an > of_graph with the Type-C controller. > > > I really would like for us to come up with a way where the core can > notify the glue that role switching is occurring, so that we instead of > adding more duplication could aim to, over time, remove the extcon and > usb_role_switch logic from the Qualcomm, OMAP and Meson drivers. We can make a comparison between clk rate notifiers. Anyone can subscribe to a clk rate notification and react to the notification. A generic dual role notification system should allow for something similar. I really don't get why folks want to treat a glue and core driver differently in this case. Why do we want to pass function pointers around instead of letting whatever role notification mechanism to be able to notify more than one user? Also keep in mind that we have dwc3 implementations which are dual role capable but don't ship the synopsys DRD block. Rather, there's a peripheral-only dwc3 instance and a separate xhci with custom logic handling role swap. If we were to provide a dwc3-specific role swap function-pointer based interface, we would just create yet another special case for this. A better approach would be to start consolidating all of these different role-swap mechanisms in a generic layer that "knows-it-all". If dwc3 is generating the role notification or a separate type-c controller or even some EC IRQ, that shouldn't matter for the listeners.
On Wed 25 Aug 01:18 PDT 2021, Bryan O'Donoghue wrote: > On 25/08/2021 06:51, Felipe Balbi wrote: > > > Hi Peter, I took a proper look at this again, hoping to find a way to > > > pass a callback pointer from dwc3-qcom to the dwc3 core, that can be > > > called from __dwc3_set_mode() to inform the Qualcomm glue about mode > > > changes. > > I would rather keep the strict separation between glue and core. > > # 1 __dwc3_set_mode > Felipe wants to keep a strict separation between core and glue > > # notifier > Requires the core probe() to complete before the glue probe to work > reliably. This then would lead us to remaking the dwc3-qcom::probe() to > facilitate probe deferral. > > We can be sure bugs would be introduced in this process. > > AFAIK Felipe is not opposed to this, Bjorn likes it > Using a notifier or just a direct callback from core to the glue is an implementation detail, but as you say we need a way for the glue to register this before the core is fully probed. > # 2 extcon > Works but a) is deprecated and b) even if it weren't deprecated has no way > to layer the messages - that I know of. > Even with extcon, I really don't fancy the fact that we're duplicating extcon registration in the glue and core - not to mention how it looks in DT. > # 3 USB role switch > Already in-place for the producer {phy, type-c port, usb-gpio typec, google > ecros} to consumer dwc-core. It already has a layering 1:1 of that array of > producers to the consumer. > > Unlike extcon though it cannot relay messages to more than one consumer. > > As I see it we can either > > A. Rewrite the dwc3-qcom probe to make it synchronous with dwc3-core probe > taking the hit of whatever bugs get thrown up as a result of that over the > next while, potentially years. > The reason for it to be synchronous is that we need the glue to be able to register it in a way that the core can acquire it when it probes later. > B. Use USB role switch in some format. > > Either > X. as I've submitted here based on a bit of code in dwc3-core or > > Y. maybe trying to hide the "relay" aspect in DTS and USB role-switch core > I don't think it's appropriate to work around the split model in DT. > It seems to me our choices are notifier + pain and churn - perhaps low, > perhaps high or USB role switch > > 3.B.X works and is what has been submitted here but, if it is objectionable > is 3.B.Y viable ? > > As in make USB role switch propigate to multiple consumers via DTS and > whatever additional work is required in the role-switch layer ? > > + Heikki on that one. > I've not seen the need for multiple consumer of role switching yet (I don't find this a legit target for it). But in the case of Type-C altmode several of our boards have either an external gpio-based SBU-pin-swapper or some redriver on I2C with this functionality, so we need a way to tell both the PHY and this external contraption about the orientation. Regards, Bjorn
On Wed 25 Aug 08:22 PDT 2021, Felipe Balbi wrote: > > Hi, > > Bjorn Andersson <bjorn.andersson@linaro.org> writes: > >> Bjorn Andersson <bjorn.andersson@linaro.org> writes: > >> > On Wed 07 Jul 20:06 PDT 2021, Peter Chen wrote: > >> > > >> >> On 21-07-07 14:03:19, Bjorn Andersson wrote: > >> >> > On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote: > >> >> > > >> >> > Allow me to reorder your two questions: > >> >> > > >> >> > > And why using a notifier need to concern core's deferral probe? > >> >> > > >> >> > The problem at hand calls for the core for somehow invoking > >> >> > dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed. > >> >> > > >> >> > This means that dwc3-qcom somehow needs to inform the dwc3-core about > >> >> > this (and stash the pointer). And this can't be done until dwc3-core > >> >> > actually exist, which it won't until dwc3_probe() has completed > >> >> > successfully (or in particular allocated struct dwc). > >> >> > >> >> Maybe you misunderstood the notifier I meant previous, my pointer was > >> >> calling glue layer API directly. > >> >> > >> >> Role switch is from dwc3-core, when it occurs, it means structure dwc3 has > >> >> allocated successfully, you could call glue layer notifier at function > >> >> dwc3_usb_role_switch_set directly. > >> >> Some references of my idea [1] [2] > >> >> > >> >> [1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event > >> >> [2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205 > >> >> > >> > > >> > Hi Peter, I took a proper look at this again, hoping to find a way to > >> > pass a callback pointer from dwc3-qcom to the dwc3 core, that can be > >> > called from __dwc3_set_mode() to inform the Qualcomm glue about mode > >> > changes. > >> > >> I would rather keep the strict separation between glue and core. > >> > > > > I'm okay with that goal, but the result is that both the OMAP and > > Qualcomm driver duplicates the extcon interface already present in the > > DRD, and the Meson driver duplicates the usb_role_switch. In addition to > > the code duplication this manifest itself in the need for us to link > > extcon to both the glue and core nodes in DeviceTree. > > > > In order to function in a USB-C based setup we now need to register a > > usb_role_switch from the Qualcomm glue and we need to evolve the > > usb_role_switch implementation to allow for the Type-C controller to > > notify more than a single role-switcher. > > > > So we're facing the need to introduce another bunch of duplication and > > the DT will be quite ugly with both glue and core having to set up an > > of_graph with the Type-C controller. > > > > > > I really would like for us to come up with a way where the core can > > notify the glue that role switching is occurring, so that we instead of > > adding more duplication could aim to, over time, remove the extcon and > > usb_role_switch logic from the Qualcomm, OMAP and Meson drivers. > > We can make a comparison between clk rate notifiers. Anyone can > subscribe to a clk rate notification and react to the notification. A > generic dual role notification system should allow for something > similar. I really don't get why folks want to treat a glue and core > driver differently in this case. > > Why do we want to pass function pointers around instead of letting > whatever role notification mechanism to be able to notify more than one > user? > > Also keep in mind that we have dwc3 implementations which are dual role > capable but don't ship the synopsys DRD block. Rather, there's a > peripheral-only dwc3 instance and a separate xhci with custom logic > handling role swap. > So you're suggesting that we invent a 3rd mechanism (in addition to the already existing duplication between extcon and usb_role_switch) for propagating role switching notifications through the kernel? > If we were to provide a dwc3-specific role swap function-pointer based > interface, we would just create yet another special case for this. A > better approach would be to start consolidating all of these different > role-swap mechanisms in a generic layer that "knows-it-all". If dwc3 is > generating the role notification or a separate type-c controller or even > some EC IRQ, that shouldn't matter for the listeners. > I was under the impression that usb_role_switch is the attempt to replace extcon as the one solution. The problem in the dwc3 case is that the same piece of hardware (i.e. _the_ USB controller) needs to implement and wired up as two separate consumers of that message. I recognize the complexity caused by the flexibility in DWC3 based designs, but I would like to see whatever combination be seen as a single entity to the rest of the system - rather than building the notification plumbing between the two pieces using DeviceTree. Regards, Bjorn
On Wed 25 Aug 09:43 PDT 2021, Heikki Krogerus wrote: > On Wed, Aug 25, 2021 at 08:53:29AM -0700, Bjorn Andersson wrote: > > On Wed 25 Aug 01:18 PDT 2021, Bryan O'Donoghue wrote: > > > > > On 25/08/2021 06:51, Felipe Balbi wrote: > > > > > Hi Peter, I took a proper look at this again, hoping to find a way to > > > > > pass a callback pointer from dwc3-qcom to the dwc3 core, that can be > > > > > called from __dwc3_set_mode() to inform the Qualcomm glue about mode > > > > > changes. > > > > I would rather keep the strict separation between glue and core. > > > > > > # 1 __dwc3_set_mode > > > Felipe wants to keep a strict separation between core and glue > > > > > > # notifier > > > Requires the core probe() to complete before the glue probe to work > > > reliably. This then would lead us to remaking the dwc3-qcom::probe() to > > > facilitate probe deferral. > > > > > > We can be sure bugs would be introduced in this process. > > > > > > AFAIK Felipe is not opposed to this, Bjorn likes it > > Notifiers were proposed for the USB role switches already some time > ago [1], and I don't think anybody was against them, but in the end I > don't think there were any users for those notifier, so they were > never added. > > If something needs to only react to the role changes like I think in > this case, then I would just add those notifiers to the USB role > switches. > > [1] https://lore.kernel.org/linux-usb/20191002231617.3670-3-john.stultz@linaro.org/ > Afaict this would end up pretty much identical to the notification chain that Bryan proposed earlier; the dwc3 drd code registers a usb_role_switch and the glue code somehow needs to get hold of that resource to register the notification. But the glue code has no way to know when the core/drd code is done registering, so it has no way to know when there is a notification chain to register with. Regards, Bjorn
Hi, Bjorn Andersson <bjorn.andersson@linaro.org> writes: > On Wed 25 Aug 10:59 PDT 2021, Bryan O'Donoghue wrote: > >> On 25/08/2021 16:53, Bjorn Andersson wrote: >> > But in the case of Type-C altmode several of our boards have either an >> > external gpio-based SBU-pin-swapper or some redriver on I2C with this >> > functionality, so we need a way to tell both the PHY and this external >> > contraption about the orientation. >> >> Its a very similar problem to orientation switch >> >> As an example >> >> - redriver may need to fix up signal integrity for >> lane switching >> >> - PHY needs to toggle lanes from one IP block to another >> > > Right, conceptually the problem is similar, but IMHO there's a big > difference in that the redriver and PHY are two physically separate > entities - on different buses. The dwc3 glue and core represent the same > piece of hardware. no they don't. The glue is a real piece of HW that adapts the "generic" synopsys IP to a given SoC. OMAP, for example, was adapting Synopsys' proprietary interface to the Sonics interconnect, while some others may adapt it to AXI or PCI or whatever. They are different HW blocks, the glue (in many cases) has its own IRQ, its own address space, its own register file, and so on. Granted, the glue also serves as an access port from CPU to the synopsys core, but that's handled by `ranges' DT property.
On Wed, Sep 15, 2021 at 08:53:35AM -0500, Bjorn Andersson wrote: > On Thu 26 Aug 01:15 CDT 2021, Felipe Balbi wrote: > > > > > Hi, > > > > Bjorn Andersson <bjorn.andersson@linaro.org> writes: > > > On Wed 25 Aug 10:59 PDT 2021, Bryan O'Donoghue wrote: > > > > > >> On 25/08/2021 16:53, Bjorn Andersson wrote: > > >> > But in the case of Type-C altmode several of our boards have either an > > >> > external gpio-based SBU-pin-swapper or some redriver on I2C with this > > >> > functionality, so we need a way to tell both the PHY and this external > > >> > contraption about the orientation. > > >> > > >> Its a very similar problem to orientation switch > > >> > > >> As an example > > >> > > >> - redriver may need to fix up signal integrity for > > >> lane switching > > >> > > >> - PHY needs to toggle lanes from one IP block to another > > >> > > > > > > Right, conceptually the problem is similar, but IMHO there's a big > > > difference in that the redriver and PHY are two physically separate > > > entities - on different buses. The dwc3 glue and core represent the same > > > piece of hardware. > > > > no they don't. The glue is a real piece of HW that adapts the "generic" > > synopsys IP to a given SoC. OMAP, for example, was adapting Synopsys' > > proprietary interface to the Sonics interconnect, while some others may > > adapt it to AXI or PCI or whatever. > > > > They are different HW blocks, the glue (in many cases) has its own IRQ, > > its own address space, its own register file, and so on. Granted, the > > glue also serves as an access port from CPU to the synopsys core, but > > that's handled by `ranges' DT property. > > > > So what you're saying is that the "Qualcomm glue" is the hardware, and > the core is just the basis for that design? > > Regardless, on the Qualcomm platforms we have need to inform both > devices about role changes, how do we do this? (Without platform_data > and preferably without having to duplicate the typec code in the glue > and core and the two device nodes in DT) That part can be handled by simply adding the notifiers to the USB role switch class as said before [1], so I think the actual problem here is that your glue layer depends on core that your glue creates (or a resource that the core driver creates). I think the dependency on dwc3 core, and what ever it creates, issue you should be able to solve by using the component framework. I'll attach you a patch showing how it should probable look like (not even compile tested). So the dwc3 core is the aggregate driver and the glue is the only component in that example (that should be enough for your needs), but it should not be any problem to later register also the child devices (xHCI, USB role switch, etc.) as components if needed. [1] https://lore.kernel.org/linux-usb/20191002231617.3670-3-john.stultz@linaro.org/ thanks, -- heikki diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 0104a80b185e1..7cc49611af74f 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -26,6 +26,7 @@ #include <linux/acpi.h> #include <linux/pinctrl/consumer.h> #include <linux/reset.h> +#include <linux/component.h> #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> @@ -1521,11 +1522,32 @@ static void dwc3_check_params(struct dwc3 *dwc) } } +static int dwc3_aggregate_bind(struct device *dev) +{ + return component_bind_all(dev, NULL); +} + +static void dwc3_aggregate_unbind(struct device *dev) +{ + component_unbind_all(dev, NULL); +} + +static const struct component_master_ops dwc3_aggregate_ops = { + .bind = dwc3_aggregate_bind, + .unbind = dwc3_aggregate_unbind, +}; + +static int dwc3_compare(struct device *dev, void *data) +{ + return dev == data; +} + static int dwc3_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct resource *res, dwc_res; struct dwc3 *dwc; + struct component_match *match = NULL; int ret; @@ -1646,10 +1668,21 @@ static int dwc3_probe(struct platform_device *pdev) if (ret) goto err5; + /* Add component match entry for the glue. */ + component_match_add(dwc->dev, &match, dwc3_compare, dwc->dev->parent); + + /* Everything is ready now. Declare this driver as the aggregate. */ + ret = component_master_add_with_match(dwc->dev, &dwc3_aggregate_ops, match); + if (ret) + goto err6; + pm_runtime_put(dev); return 0; +err6: + dwc3_core_exit_mode(dwc); + err5: dwc3_debugfs_exit(dwc); dwc3_event_buffers_cleanup(dwc); @@ -1696,6 +1729,8 @@ static int dwc3_remove(struct platform_device *pdev) pm_runtime_get_sync(&pdev->dev); + component_master_del(dwc->dev, &dwc3_aggregate_ops); + dwc3_core_exit_mode(dwc); dwc3_debugfs_exit(dwc); diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 9abbd01028c5f..ffe585344d6a8 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -20,6 +20,8 @@ #include <linux/usb/of.h> #include <linux/reset.h> #include <linux/iopoll.h> +#include <linux/usb/role.h> +#include <linux/component.h> #include "core.h" @@ -81,6 +83,7 @@ struct dwc3_qcom { struct extcon_dev *host_edev; struct notifier_block vbus_nb; struct notifier_block host_nb; + struct notifier_block role_nb; const struct dwc3_acpi_pdata *acpi_pdata; @@ -717,6 +720,51 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev) return acpi_create_platform_device(adev, NULL); } +static int dwc3_qcom_role_notifier(struct notifier_block *nb, + unsigned long event, void *ptr) +{ + struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom, role_nb); + + /* Do what ever you need to do... */ + + return NOTIFY_DONE; +} + +static int dwc3_qcom_bind(struct device *dev, struct device *dwc, void *data) +{ + struct usb_role_switch *sw = usb_role_switch_find_by_fwnode(dev_fwnode(dwc)); + struct dwc3_qcom *qcom = dev_get_drvdata(dev); + + /* + * Time to finalize initilization. + * + * Our aggregate driver - dwc3 core - is guaranteed to be ready when + * this is called. That means USB role switch "sw" is also now ready. + */ + + /* Register role switch notifier */ + qcom->role_nb.notifier_call = dwc3_qcom_role_notifier; + usb_role_switch_register_notifier(sw, qcom->role_nb); + usb_role_switch_put(sw); + + return 0; +} + +static void dwc3_qcom_unbind(struct device *dev, struct device *dwc, void *data) +{ + struct usb_role_switch *sw = usb_role_switch_find_by_fwnode(dev_fwnode(dwc)); + struct dwc3_qcom *qcom = dev_get_drvdata(dev); + + /* Unregister role switch notifier */ + usb_role_switch_unregister_notifier(sw, qcom->role_nb); + usb_role_switch_put(sw); +} + +static const struct component_ops dwc3_qcom_component_ops = { + .bind = dwc3_qcom_bind, + .unbind = dwc3_qcom_unbind, +}; + static int dwc3_qcom_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; @@ -837,6 +885,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev) if (ret) goto interconnect_exit; + ret = component_add(dev, &dwc3_qcom_component_ops); + if (ret) + goto interconnect_exit; + device_init_wakeup(&pdev->dev, 1); qcom->is_suspended = false; pm_runtime_set_active(dev); @@ -869,6 +921,7 @@ static int dwc3_qcom_remove(struct platform_device *pdev) struct device *dev = &pdev->dev; int i; + component_del(dev, &dwc3_qcom_component_ops); device_remove_software_node(&qcom->dwc3->dev); of_platform_depopulate(dev);
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. 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(-)