Date:   Sat, 13 Apr 2019 22:38:49 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Adam Thomson <Adam.Thomson.Opensource@...semi.com>,
        Kyle Tso <kyletso@...gle.com>
Cc:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Greg KH <gregkh@...uxfoundation.org>,
        Badhri Jagan Sridharan <badhri@...gle.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance

Hi,

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.

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

Powered by blists - more mailing lists