Re: [PATCH 1/2] usb: dwc3: dwc3-qcom: Find USB connector and register role switch

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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