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