Re: [PATCH v2] usb: typec: tcpm: collision avoidance

[Guenter Roeck]

On 9/19/19 3:48 AM, Kyle Tso wrote:
> Ping! Anyone still reviewing this patch?

Which patch ? Hans' series was long since pushed.

Guenter

> I have another change related to AMS.
> I will group them as a set and re-send it.
>
> Regards,
> Kyle Tso
>
>
> On Mon, Apr 15, 2019 at 6:03 PM Adam Thomson
> <Adam.Thomson.Opensource@xxxxxxxxxxx> wrote:
> > On 13 April 2019 21:39, Hans de Goede wrote:
> >
> > > On 10-04-19 18:38, Hans de Goede wrote:
> > > > On 10-04-19 18:14, Adam Thomson wrote:
> > > > > On 10 April 2019 16:45, Hans de Goede wrote:
> > >
> > > <snip>
> > >
> > > > > > Starting toggling from tcpm_set_cc() just feels wrong; and currently
> > > > > > power role swapping is broken with the fusb302, which IIRC used to
> > > > > > work. I suspect this is related.
> > > > > >
> > > > > > I plan to write a patch tomorrow to functionally take tcpm_set_cc()
> > > > > > back to the way it was before. This should fix your case and I hope
> > > > > > this also fixes power-role swapping.
> > > > > >
> > > > > > This will re-introduce Adam Thomson's problem, but I have a feeling
> > > > > > that that actually needs a fix in the tcpm.c code rather then at the fusb302
> > > level.
> > > > > To be clear here, the names TOGGLING_MODE_SNK and
> > > TOGGLING_MODE_SRC
> > > > > are a misnomer from the HW spec for fusb302. The device isn't
> > > > > toggling anything as far as I'm aware, so I don't necessarily agree with your
> > > point.
> > > > If I understand the datasheet correctly:
> > > >
> > > > "The FUSB302 has the capability to do autonomous DRP toggle. In
> > > > autonomous toggle the FUSB302 internally controls the PDWN1, PDWN2,
> > > > PU_EN1 and PU_EN2, MEAS_CC1 and MEAS_CC2 and implements a fixed DRP
> > > > toggle between presenting as a SRC and presenting as a SNK.
> > > > Alternately, it can present as a SRC or SNK only and poll CC1 and CC2
> > > > continuously."
> > > >
> > > > It is still attaching Rp resp Rd to CC1 or CC2 one at a time to detect
> > > > polarity, so it is still toggling, it just is not doing dual-role
> > > > toggling. This is also expected behavior for a sink, a sink may not
> > > > present Rd on both CC pins at the same time, otherwise the source
> > > > cannot detect the polarity and the source also cannot detect if Vconn
> > > > is necessary.
> > > >
> > > > > It's a mechanism to
> > > > > have the HW report when the CC line changes on connection. Without
> > > > > that we have no reporting from the HW for the fixed role scenarios.
> > > >
> > > > Not just connection, also polarity detection. Notice that the tcpm
> > > > framework / the driver also has a start_drp_toggling() method. I think
> > > > we may also need a start_srp_toggling function just like it and call
> > > > that from the SNK_UNATTACHED and SRC_UNATTACHED states for single-role
> > > > ports. I agree that we need to start toggling when in those states,
> > > > but tcpm_set_cc gets called in a lot of other places where AFAIK we
> > > > should NOT restart toggling and your patch causes us to restart
> > > > toggling in those cases.
> > >
> > > Ok, so as I suspected, commit ea3b4d5523bc ("usb: typec: fusb302:
> > > Resolve fixed power role contract setup") is what caused the power-role
> > > swapping breakage I've been seeing.
> >
> > Apologies for the breakage. Annoyed I didn't catch that when submitting that
> > patch. Thanks for looking to resolve this and will review your updates shortly.
> >
> > > So I've prepared a 3 patch series:
> > >
> > > 1) Add a new start_srp_connection_detect function which, when implemented
> > > by the tcpc_dev, gets called instead of start_drp_toggling for single role ports
> > > (SRPs)
> > >
> > > 2) Implement 1. for fusb302 to fix the SRP issue Adam was seeing, without
> > > depending on set_cc starting "toggling"
> > > or something like it for the fix
> > >
> > > 3) Revert commit ea3b4d5523bc, restoring power-role swap functionality.
> > >
> > > This should also fix the issue Kyle Tso was seeing when trying to change from one
> > > Rp setting to another.
> > >
> > > I'll send out the series right after this email.
> > >
> > > Regards,
> > >
> > > Hans