| Message ID | 20210629144449.2550737-1-bryan.odonoghue@linaro.org |
|---|---|
| Headers | show |
| Series | dwc3-qcom: Prepare the ground for pm8150b tcpm | expand |
On Tue 29 Jun 09:44 CDT 2021, Bryan O'Donoghue wrote: > From: Wesley Cheng <wcheng@codeaurora.org> > > If registering a USB typeC connector, the connector node may not be a child > of the DWC3 QCOM device. Utilize devcon graph search to lookup if any > remote endpoints contain the connector. If a connector is present, the > DWC3 QCOM will register a USB role switch to receive role change events, as > well as attain a reference to the DWC3 core role switch to pass the event > down. > What's wrong with the switch that dwc3_setup_role_switch() sets up? That's what I've been using in my UCSI hacking on Snapdragon 888 and it seems to work... Regards, Bjorn > Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> > Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > drivers/usb/dwc3/dwc3-qcom.c | 118 ++++++++++++++++++++++++++++++++++- > 1 file changed, 116 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 49e6ca94486d..4491704503ab 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/fwnode.h> > +#include <linux/usb/role.h> > > #include "core.h" > > @@ -82,6 +84,9 @@ struct dwc3_qcom { > struct notifier_block vbus_nb; > struct notifier_block host_nb; > > + struct usb_role_switch *role_sw; > + struct usb_role_switch *dwc3_drd_sw; > + > const struct dwc3_acpi_pdata *acpi_pdata; > > enum usb_dr_mode mode; > @@ -296,6 +301,73 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) > icc_put(qcom->icc_path_apps); > } > > +static int dwc3_qcom_usb_role_switch_set(struct usb_role_switch *sw, > + enum usb_role role) > +{ > + struct dwc3_qcom *qcom = usb_role_switch_get_drvdata(sw); > + struct fwnode_handle *child; > + bool enable = false; > + > + if (!qcom->dwc3_drd_sw) { > + child = device_get_next_child_node(qcom->dev, NULL); > + if (child) { > + qcom->dwc3_drd_sw = usb_role_switch_find_by_fwnode(child); > + fwnode_handle_put(child); > + if (IS_ERR(qcom->dwc3_drd_sw)) { > + qcom->dwc3_drd_sw = NULL; > + return 0; > + } > + } > + } > + > + usb_role_switch_set_role(qcom->dwc3_drd_sw, role); > + > + if (role == USB_ROLE_DEVICE) > + enable = true; > + else > + enable = false; > + > + qcom->mode = (role == USB_ROLE_HOST) ? USB_DR_MODE_HOST : > + USB_DR_MODE_PERIPHERAL; > + dwc3_qcom_vbus_overrride_enable(qcom, enable); > + return 0; > +} > + > +static enum usb_role dwc3_qcom_usb_role_switch_get(struct usb_role_switch *sw) > +{ > + struct dwc3_qcom *qcom = usb_role_switch_get_drvdata(sw); > + enum usb_role role; > + > + switch (qcom->mode) { > + case USB_DR_MODE_HOST: > + role = USB_ROLE_HOST; > + break; > + case USB_DR_MODE_PERIPHERAL: > + role = USB_ROLE_DEVICE; > + break; > + default: > + role = USB_ROLE_DEVICE; > + break; > + } > + > + return role; > +} > + > +static int dwc3_qcom_setup_role_switch(struct dwc3_qcom *qcom) > +{ > + struct usb_role_switch_desc dwc3_role_switch = {NULL}; > + > + dwc3_role_switch.fwnode = dev_fwnode(qcom->dev); > + dwc3_role_switch.set = dwc3_qcom_usb_role_switch_set; > + dwc3_role_switch.get = dwc3_qcom_usb_role_switch_get; > + dwc3_role_switch.driver_data = qcom; > + qcom->role_sw = usb_role_switch_register(qcom->dev, &dwc3_role_switch); > + if (IS_ERR(qcom->role_sw)) > + return PTR_ERR(qcom->role_sw); > + > + return 0; > +} > + > static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom) > { > if (qcom->hs_phy_irq) { > @@ -698,6 +770,40 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev) > return acpi_create_platform_device(adev, NULL); > } > > +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; > +} > + > +static void *dwc3_qcom_find_usb_connector_match(struct fwnode_handle *fwnode, > + const char *id, void *data) > +{ > + /* Check if the "connector" node is the parent of the remote endpoint */ > + if (dwc3_qcom_connector_check(fwnode)) > + return fwnode; > + > + /* else, check if it is a child node */ > + fwnode = fwnode_get_named_child_node(fwnode, "connector"); > + if (dwc3_qcom_connector_check(fwnode)) > + return fwnode; > + > + return 0; > +} > + > +static bool dwc3_qcom_find_usb_connector(struct platform_device *pdev) > +{ > + struct fwnode_handle *fwnode = pdev->dev.fwnode; > + > + return fwnode_connection_find_match(fwnode, "connector", NULL, > + dwc3_qcom_find_usb_connector_match); > +} > + > static int dwc3_qcom_probe(struct platform_device *pdev) > { > struct device_node *np = pdev->dev.of_node; > @@ -813,8 +919,13 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > if (qcom->mode == USB_DR_MODE_PERIPHERAL) > dwc3_qcom_vbus_overrride_enable(qcom, true); > > - /* register extcon to override sw_vbus on Vbus change later */ > - ret = dwc3_qcom_register_extcon(qcom); > + if (dwc3_qcom_find_usb_connector(pdev)) { > + ret = dwc3_qcom_setup_role_switch(qcom); > + } else { > + /* register extcon to override sw_vbus on Vbus change later */ > + ret = dwc3_qcom_register_extcon(qcom); > + } > + > if (ret) > goto interconnect_exit; > > @@ -850,6 +961,9 @@ static int dwc3_qcom_remove(struct platform_device *pdev) > struct device *dev = &pdev->dev; > int i; > > + usb_role_switch_unregister(qcom->role_sw); > + usb_role_switch_put(qcom->dwc3_drd_sw); > + > device_remove_software_node(&qcom->dwc3->dev); > of_platform_depopulate(dev); > > -- > 2.30.1 >
On Tue, Jun 29, 2021 at 08:23:46PM +0100, Bryan O'Donoghue wrote: > On 29/06/2021 16:48, Bjorn Andersson wrote: > > What's wrong with the switch that dwc3_setup_role_switch() sets up? > > > > That's what I've been using in my UCSI hacking on Snapdragon 888 and it > > seems to work... Bjorn are you exercising dual-role or just host mode? > A good question, which as soon as you asked it made me completely doubt if > I'd tested the tree without the patch applied. > > I reverted both on my working tree and indeed it breaks role-switch > detection. > > In TCPM the connector is a child node of TCPM > > https://git.linaro.org/people/bryan.odonoghue/kernel.git/tree/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts?h=usb-next-5.13.rcx-rb5-tcpm > line 1396 > > We use a remote endpoint inside of TCPM to hook up into &usb_1 line 1303 > > Not entirely sure what the graph tree looks like in your USCI case but I > guess the connector is a child node of the controller ? > > But I think your question is why bother with the role-switch in dwc3-qcom > > dwc3_qcom_vbus_override_enable(){} is switching bits that probably ought to > be in the PHY but for whatever reason ended up being buried in the qcom-dwc3 > wrapper. This. When switching dwc3 into peripheral mode we also need dwc3_qcom_vbus_override_enable() to write those registers to manually drive the UTMI VBUS valid signal high to the controller since our SoCs are never physically wired to the connector's VBUS. These registers (QSCRATCH_{HS,SS}_PHY_CTRL) however don't belong to the PHYs but are part of our dwc3 wrapper's IO map and hence dwc3-qcom is the only appropriate place to handle them since dwc3-qcom has over the years paired with several different PHYs depending on SoC. Wesley's approach in $SUBJECT was to designate dwc3-qcom itself as a usb-role-switch-able device so that in the DT the connector graph endpoints would wire to the dwc3-qcom device itself instead of its dwc3 core child. The idea would be for dwc3-qcom would intercept the role_switch_set call from TCPM/UCSI, call the vbus_override_enable() and then pass on the notification to the child to let dwc3/drd.c handle the rest. IIRC there had been an alternate proposal to keep the role switch connection only at the dwc3 core but in order to handle the vbus override business an additional notification would have needed to be done either from the usb_role_switch class driver or as an "upcall" from dwc3 core -> glue. (found it, it was by you Bryan [1]) [1] https://lore.kernel.org/linux-usb/20200311191501.8165-7-bryan.odonoghue@linaro.org/ Jack
On Tue 29 Jun 14:23 CDT 2021, Bryan O'Donoghue wrote: > On 29/06/2021 16:48, Bjorn Andersson wrote: > > What's wrong with the switch that dwc3_setup_role_switch() sets up? > > > > That's what I've been using in my UCSI hacking on Snapdragon 888 and it > > seems to work... > > > > Regards, > > Bjorn > > A good question, which as soon as you asked it made me completely doubt if > I'd tested the tree without the patch applied. > > I reverted both on my working tree and indeed it breaks role-switch > detection. > > In TCPM the connector is a child node of TCPM > > https://git.linaro.org/people/bryan.odonoghue/kernel.git/tree/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts?h=usb-next-5.13.rcx-rb5-tcpm > line 1396 > > We use a remote endpoint inside of TCPM to hook up into &usb_1 line 1303 > That's expected, because the dwc3 role switching logic registers on &usb_1_dwc3 instead of &usb_1. > Not entirely sure what the graph tree looks like in your USCI case but I > guess the connector is a child node of the controller ? > No, the connector currently a child of a completely different rpmsg device, i.e. in system terms it's the same as your design. One thing detecting the change in parameters and then invoking the change on the dwc3 controller (not the qcom variant thereof). > But I think your question is why bother with the role-switch in dwc3-qcom > > dwc3_qcom_vbus_override_enable(){} is switching bits that probably ought to > be in the PHY but for whatever reason ended up being buried in the qcom-dwc3 > wrapper. > I missed this part when I wired up my role switcher and things works fine, but I presume there's a good reason for the vbus override and it's not unlikely that I missed something in my testing. I do however think that it would be appropriate to use the generic dwc3 role switcher and then make that somehow poke the vbus_override in the qcom instance. > Certainly we want that for qcs405 if you remember - I'm assuming for the > sm8x50 too. > > Even if we shouldn't twiddle these bits on sm8x50 I believe its wanted on > qcs405. > Can you remind me about the purpose of these bits? Regards, Bjorn > I'm open to correction on that one though > > --- > bod
On Tue 29 Jun 15:02 CDT 2021, Jack Pham wrote: > On Tue, Jun 29, 2021 at 08:23:46PM +0100, Bryan O'Donoghue wrote: > > On 29/06/2021 16:48, Bjorn Andersson wrote: > > > What's wrong with the switch that dwc3_setup_role_switch() sets up? > > > > > > That's what I've been using in my UCSI hacking on Snapdragon 888 and it > > > seems to work... > > Bjorn are you exercising dual-role or just host mode? > I remember testing switching between host and peripheral, but given your explanation below I doubt that I actually validated that peripheral mode was functional. > > A good question, which as soon as you asked it made me completely doubt if > > I'd tested the tree without the patch applied. > > > > I reverted both on my working tree and indeed it breaks role-switch > > detection. > > > > In TCPM the connector is a child node of TCPM > > > > https://git.linaro.org/people/bryan.odonoghue/kernel.git/tree/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts?h=usb-next-5.13.rcx-rb5-tcpm > > line 1396 > > > > We use a remote endpoint inside of TCPM to hook up into &usb_1 line 1303 > > > > Not entirely sure what the graph tree looks like in your USCI case but I > > guess the connector is a child node of the controller ? > > > > But I think your question is why bother with the role-switch in dwc3-qcom > > > > dwc3_qcom_vbus_override_enable(){} is switching bits that probably ought to > > be in the PHY but for whatever reason ended up being buried in the qcom-dwc3 > > wrapper. > > This. When switching dwc3 into peripheral mode we also need > dwc3_qcom_vbus_override_enable() to write those registers to manually > drive the UTMI VBUS valid signal high to the controller since our SoCs > are never physically wired to the connector's VBUS. These registers > (QSCRATCH_{HS,SS}_PHY_CTRL) however don't belong to the PHYs but are > part of our dwc3 wrapper's IO map and hence dwc3-qcom is the only > appropriate place to handle them since dwc3-qcom has over the years > paired with several different PHYs depending on SoC. > > Wesley's approach in $SUBJECT was to designate dwc3-qcom itself as a > usb-role-switch-able device so that in the DT the connector graph > endpoints would wire to the dwc3-qcom device itself instead of its dwc3 > core child. The idea would be for dwc3-qcom would intercept the > role_switch_set call from TCPM/UCSI, call the vbus_override_enable() and > then pass on the notification to the child to let dwc3/drd.c handle the > rest. > I really do object against us adding another role switch implementation just to, once again, circumvent the annoying split between the platform specific and core part of the dwc3 driver. > IIRC there had been an alternate proposal to keep the role switch > connection only at the dwc3 core but in order to handle the vbus > override business an additional notification would have needed to be > done either from the usb_role_switch class driver or as an "upcall" from > dwc3 core -> glue. (found it, it was by you Bryan [1]) > > [1] https://lore.kernel.org/linux-usb/20200311191501.8165-7-bryan.odonoghue@linaro.org/ I liked this, and it worked when I tested it, but iirc it suffered from the problem that the core's probe may or may not have finished successfully at the time that of_platform_populate() returns. But fixing this problem would save us quite a bit of headache. Regards, Bjorn
On 29/06/2021 22:57, Bryan O'Donoghue wrote: > On 29/06/2021 21:30, Bjorn Andersson wrote: >> I liked this, and it worked when I tested it, but iirc it suffered from >> the problem that the core's probe may or may not have finished >> successfully at the time that of_platform_populate() returns. >> >> But fixing this problem would save us quite a bit of headache. > > OK. > > I will take a look at resurrecting the old patches either fixing the > probe order - or perhaps using something like Wesley's role-switch to > have dwc3 core optionally trigger dwc3-qcom > > Binding tcpm into &usb_1_dwc3 instead of &usb_1 > > --- > bod So here's a potential way forward. Not technically breaking my "no patches at 3am rule" Basically we can fix the probe order problem if we have dwc3 drd call into dwc3-qcom. In order to make that not be a problem for all non qcom platforms - use a function with weak binding in drd - which a wrapper - in this case the qcom wrapper can over ride.. --- bod From 329ffacea542163fe7ac798b77db7cb3599a65ac Mon Sep 17 00:00:00 2001 From: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Date: Wed, 11 Mar 2020 19:15:00 +0000 Subject: [PATCH 1/2] usb: dwc3: Add support for a role-switch notifier Role-switching is a 1:1 mapping between a producer and a consumer. For DWC3 we have some vendor specific wrappers, notably the qcom wrapper that want to toggle some PHY related bits on a USB role switch. This patch adds a role-switch notifier to the dwc3 drd code. When the USB role-switch set() routine runs, the notifier will fire passing the notified mode to the consumer, thus allowing vendor specific fix-ups to toggle from the role-switching events. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/usb/dwc3/core.h | 8 ++++++++ drivers/usb/dwc3/drd.c | 5 +++++ 2 files changed, 13 insertions(+) diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index dccdf13b5f9e..7f81ee3a9657 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -995,6 +995,7 @@ struct dwc3_scratchpad_array { * @role_sw: usb_role_switch handle * @role_switch_default_mode: default operation mode of controller while * usb role is USB_ROLE_NONE. + * @role_sw_nl: role switch notifier list * @usb_psy: pointer to power supply interface. * @usb2_phy: pointer to USB2 PHY * @usb3_phy: pointer to USB3 PHY @@ -1136,6 +1137,7 @@ struct dwc3 { struct notifier_block edev_nb; enum usb_phy_interface hsphy_mode; struct usb_role_switch *role_sw; + struct raw_notifier_head role_sw_nl; enum usb_dr_mode role_switch_default_mode; struct power_supply *usb_psy; @@ -1586,4 +1588,10 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc) { } #endif +#if IS_ENABLED(CONFIG_USB_ROLE_SWITCH) +void __weak +dwc3_set_parent_role_switch_notifier(struct device *parent, + struct raw_notifier_head *role_sw_nl) {} +#endif + #endif /* __DRIVERS_USB_DWC3_CORE_H */ diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c index 8fcbac10510c..7ae09730a319 100644 --- a/drivers/usb/dwc3/drd.c +++ b/drivers/usb/dwc3/drd.c @@ -507,6 +507,8 @@ static int dwc3_usb_role_switch_set(struct usb_role_switch *sw, } dwc3_set_mode(dwc, mode); + raw_notifier_call_chain(&dwc->role_sw_nl, mode, NULL); + return 0; } @@ -563,6 +565,9 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc) if (IS_ERR(dwc->role_sw)) return PTR_ERR(dwc->role_sw); + RAW_INIT_NOTIFIER_HEAD(&dwc->role_sw_nl); + dwc3_set_parent_role_switch_notifier(dwc->dev->parent, + &dwc->role_sw_nl); dwc3_set_mode(dwc, mode); return 0; } -- 2.30.1 From 9d2b50b448f7efd8b3891a2e8f52440794ed2958 Mon Sep 17 00:00:00 2001 From: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Date: Wed, 30 Jun 2021 03:10:49 +0100 Subject: [PATCH 2/2] usb: dwc3: qcom: Implement VBUS role-switch notifier hooks Implement dwc3_set_parent_role_switch_notifier() in dwc3-qcom allowing the core drd code to call into the parent device when it has successfully setup all necessary role-switching logic. The qcom-dwc3 binding reuses the existing VBUS notifier for extcon receiving notifications from the role-switch layer instead of extcon. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/usb/dwc3/dwc3-qcom.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 49e6ca94486d..1b5aa345d025 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -81,6 +81,7 @@ struct dwc3_qcom { struct extcon_dev *host_edev; struct notifier_block vbus_nb; struct notifier_block host_nb; + struct raw_notifier_head *role_sw_nl; const struct dwc3_acpi_pdata *acpi_pdata; @@ -154,6 +155,20 @@ static int dwc3_qcom_host_notifier(struct notifier_block *nb, return NOTIFY_DONE; } +#if IS_ENABLED(CONFIG_USB_ROLE_SWITCH) +void dwc3_set_parent_role_switch_notifier(struct device *dev, + struct raw_notifier_head *role_sw_nl) +{ + struct platform_device *pdev = container_of(dev, + struct platform_device, dev); + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); + + qcom->vbus_nb.notifier_call = dwc3_qcom_vbus_notifier; + if (!raw_notifier_chain_register(role_sw_nl, &qcom->vbus_nb)) + qcom->role_sw_nl = role_sw_nl; +} +#endif + static int dwc3_qcom_register_extcon(struct dwc3_qcom *qcom) { struct device *dev = qcom->dev; @@ -829,6 +844,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev) interconnect_exit: dwc3_qcom_interconnect_exit(qcom); depopulate: + if (qcom->role_sw_nl) + raw_notifier_chain_unregister(qcom->role_sw_nl, &qcom->vbus_nb); if (np) of_platform_depopulate(&pdev->dev); else @@ -850,6 +867,9 @@ static int dwc3_qcom_remove(struct platform_device *pdev) struct device *dev = &pdev->dev; int i; + if (qcom->role_sw_nl) + raw_notifier_chain_unregister(qcom->role_sw_nl, &qcom->vbus_nb); + device_remove_software_node(&qcom->dwc3->dev); of_platform_depopulate(dev); -- 2.30.1
On Wed, Jun 30, 2021 at 03:21:46AM +0100, Bryan O'Donoghue wrote: > On 29/06/2021 22:57, Bryan O'Donoghue wrote: > > On 29/06/2021 21:30, Bjorn Andersson wrote: > > > I liked this, and it worked when I tested it, but iirc it suffered from > > > the problem that the core's probe may or may not have finished > > > successfully at the time that of_platform_populate() returns. > > > > > > But fixing this problem would save us quite a bit of headache. > > > > OK. > > > > I will take a look at resurrecting the old patches either fixing the > > probe order - or perhaps using something like Wesley's role-switch to > > have dwc3 core optionally trigger dwc3-qcom > > > > Binding tcpm into &usb_1_dwc3 instead of &usb_1 > > > > --- > > bod > > So here's a potential way forward. Not technically breaking my "no patches > at 3am rule" > > Basically we can fix the probe order problem if we have dwc3 drd call into > dwc3-qcom. > > In order to make that not be a problem for all non qcom platforms - use a > function with weak binding in drd - which a wrapper - in this case the qcom > wrapper can over ride.. I'm afraid I'm not too familiar with weak symbols. Would that still work if dwc3 & dwc3-qcom are built as modules? Also is it supported with Clang/LLVM? Jack -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
On 01/07/2021 02:12, Jack Pham wrote: > I'm afraid I'm not too familiar with weak symbols. Would that still work > if dwc3 & dwc3-qcom are built as modules? Also is it supported with > Clang/LLVM? __weak would work fine, until you tried to have two strong implementations. Your linker should choke if dwc3-meson-g12a, dwc3-qcom and dwc3-drd linked together with both wrappers implementing normal linkage. However, I do think its possible to use role switching to have dwc3-drd trigger dwc3-qcom. The role switch code is resilient to deferral so we don't have to solve the problem we had with get_drvdata() in the notifier solution and it gets us out of the business of having dwc3-qcom relay the role-switch onto the core - or indeed care about what sort of connector is attached. dwc3-qcom shouldn't have to know or care what sort of connector is attached to it, ecros, gpio-typec, tcpm or a raw type-c driver and like Bjorn said, it shouldn't be the case that the wrapper relays onto the core. Anyway I'm playing with that prototype now