| Message ID | 20210629144449.2550737-2-bryan.odonoghue@linaro.org |
|---|---|
| State | New |
| 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 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 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. 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. I'm open to correction on that one though --- bod
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 -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
On 29/06/2021 21:02, Jack Pham wrote: > 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/ Which had a bug :( Bjorn found My excuse for not following up is I had to hand back my hardware and got sucked into doing something else. I think Wesley's approach here is a good one so, that's also why I'm re-posting. I don't have a functional qcs405 setup but, I have validated it on sm8250. --- bod
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 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
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
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);