Re: [RFC][PATCH 2/3] usb: roles: Add usb role switch notifier.

[Hans de Goede]

Hi,

On 18-10-2019 22:37, John Stultz wrote:
> On Fri, Oct 18, 2019 at 1:21 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> > On 18-10-2019 22:12, John Stultz wrote:
> > > On Fri, Oct 18, 2019 at 12:59 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> > > > On 18-10-2019 21:53, John Stultz wrote:
> > > > > On Fri, Oct 18, 2019 at 12:30 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> > > > > > Looking at drivers/usb/typec/tcpm/tcpci.c: tcpci_set_vconn I see that
> > > > > > there is a data struct with vendor specific callbacks and that the
> > > > > > drivers/usb/typec/tcpm/tcpci_rt1711h.c implements that.
> > > > > >
> > > > > > So you may want something similar here. But things are tricky here,
> > > > > > because when nothing is connected you want to provide Vbus for
> > > > > > the USB-A ports, which means that if someone then connects a
> > > > > > USB-A to C cable to connect the board to a PC (switching the port
> > > > > > to device mode) there will be a time when both sides are supplying
> > > > > > 5V if I remember the schedule correctly.
> > > > >
> > > > > Ok. Thanks for the pointer, I'll take a look at that to see if I can
> > > > > get it to work.
> > > > >
> > > > > > I think that the original hack might not be that bad, the whole hw
> > > > > > design seems so, erm, broken, that you probably cannot do proper
> > > > > > roleswapping anyways.  So just tying Vbus to host mode might be
> > > > > > fine, the question then becomes again how can some other piece
> > > > > > of code listen to the role-switch events...
> > > > >
> > > > > So, at least in the current approach (see the v3 series), I've
> > > > > basically set the hub driver as an role-switch intermediary, sitting
> > > > > between the calls from the tcpm to the dwc3 driver. It actually works
> > > > > better then the earlier notifier method (which had some issues with
> > > > > reliably establishing the initial state on boot).  Does that approach
> > > > > work for you?
> > > >
> > > > That sounds like it might be a nice solution. But I have not seen the
> > > > code, I think I was not Cc-ed on v3. Do you have a patchwork or
> > > > lore.kernel.org link for me?
> > >
> > > Oh! I think I had you on CC, maybe it got caught in your spam folder?
> >
> > More likely I just deleted mail to aggressively, sorry.
> >
> > > My apologies either way! The thread is here:
> > >     https://lore.kernel.org/lkml/20191016033340.1288-1-john.stultz@xxxxxxxxxx/
> > >
> > > And the hub/role-switch-intermediary driver is here:
> > >     https://lore.kernel.org/lkml/20191016033340.1288-12-john.stultz@xxxxxxxxxx/
> >
> > Hm, this looks very nice actually, much much better then the notifier stuff!
> >
> > As for your:
> >
> > "NOTE: It was noted that controlling the TYPEC_VBUS_POWER_OFF and
> > TYPEC_VBUS_POWER_ON values here is not reccomended. I'm looking
> > for a way to remove that bit from the logic here, but wanted to
> > still get feedback on this approach."
> >
> > Comment in the commit message, normally a type-c port would turn external Vbus
> > off until a sink is connected, IIRC the same Vbus is also used for the TupeA ports
> > on the hub, so that would mean those are unusable when nothing is connected to
> > the TypeC port, which is not what you want.
>
> Uh, so I think for the HiKey960, the type-A ports on the hub are
> separately powered via the hub_power_ctrl(hisi_hikey_usb,
> HUB_VBUS_POWER_OFF/ON) call.
>
> At least, with the current driver, the functionality is working as
> expected: remove the USB-C cable, and devices connected to the hub
> power on, plug something into the USB-C port and devices plugged into
> the hub shutdown.
>
> But maybe I'm missing what you mean?

Ok, so double checking the schematic I do see separate Vbus-es for the
TypeC port and the TypeA ports, with the TypeC port one being controlled
by GPIO_202_VBUS_TYPEC. So ideally that gpio would be  controlled to
enable/disable vbus by the tcpm framework.

> > So I think that given the special case / hack-ish hw you have, that just setting
> > Vbus based on the role is ok(ish).
>
> Ok. I'm happy to stick with what works here, since it is at least the
> oddness is isolated to the device specific hub driver.

Right, so for the Type-A ports Vbus controlled by PRT_CTL1 enabling it depending
on host vs devices mode makes sense. But the Type-C one really should be
controlled by the tcpm framework.

Regards,

Hans

p.s.

Sorry for the confusion I was under the impression that there was only 1
Vbus enable for both Type-A and Type-C ports.