Messages in this thread | | | Subject | Re: [PATCH v2] usb: typec: tcpm: collision avoidance | From | Guenter Roeck <> | Date | Thu, 19 Sep 2019 06:31:47 -0700 |
| |
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@diasemi.com> 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 >> >
| |