Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance
To:     Kyle Tso <kyletso@google.com>,
        Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Cc:     Hans de Goede <hdegoede@redhat.com>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Greg KH <gregkh@linuxfoundation.org>,
        Badhri Jagan Sridharan <badhri@google.com>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20190322121745.159768-1-kyletso@google.com>
 <20190404141345.GF21319@kuha.fi.intel.com>
 <08a6d422-e8f7-303e-7bf1-952344f2c182@roeck-us.net>
 <CAGZ6i=2zKLhS4By8Xanc863G+jSxPYg8SLU-UHkY94r5MiEYag@mail.gmail.com>
 <20190409130230.GC20058@kuha.fi.intel.com>
 <20190409130649.GD20058@kuha.fi.intel.com>
 <9c9d17e3-bd99-c877-359c-a0a1b10a8d73@redhat.com>
 <AM5PR1001MB099440C3AA6DA6BA2AB0F2AE802E0@AM5PR1001MB0994.EURPRD10.PROD.OUTLOOK.COM>
 <CAGZ6i=0rRgNH5bU-zcP58MNi+VSa+xeAQWL67egaZ-ui-ebmYA@mail.gmail.com>
 <9f9a2de9-2cfb-385c-8e99-54b2587113ce@redhat.com>
 <AM5PR1001MB09943830CFED9CB321CC883D802E0@AM5PR1001MB0994.EURPRD10.PROD.OUTLOOK.COM>
 <76a3c6df-63c0-78e7-c1ca-c83a30e95d38@redhat.com>
 <009662c6-2897-e2dd-03a7-992fc0a78599@redhat.com>
 <AM5PR1001MB099452876C75E45FD774BA77802B0@AM5PR1001MB0994.EURPRD10.PROD.OUTLOOK.COM>
 <CAGZ6i=10-DVWRseYXjRGVyRtnTijT9Mg_TBTkv=3qWiMfv28cw@mail.gmail.com>
From:   Guenter Roeck <linux@roeck-us.net>
Message-ID: <91295210-7eb7-7272-d6cc-e983e8bd3fdf@roeck-us.net>
Date:   Thu, 19 Sep 2019 06:31:47 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <CAGZ6i=10-DVWRseYXjRGVyRtnTijT9Mg_TBTkv=3qWiMfv28cw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
>>
>