Re: [EXT] Re: [PATCH] usb: typec: tcpci: allow drp toggling for non-drp port

[Hans de Goede <hdegoede@redhat.com>]

Hi all,

On 6/20/23 05:10, Xu Yang wrote:
> ++ Hans de Goede
> 
>> -----Original Message-----
>> From: Xu Yang
>> Sent: Friday, June 9, 2023 10:15 AM
>> To: Guenter Roeck <linux@roeck-us.net>; heikki.krogerus@linux.intel.com
>> Cc: gregkh@linuxfoundation.org; dl-linux-imx <linux-imx@nxp.com>; linux-usb@vger.kernel.org; Jun Li <jun.li@nxp.com>;
>> Zhipeng Wang <zhipeng.wang_1@nxp.com>; Faqiang Zhu <faqiang.zhu@nxp.com>
>> Subject: RE: [EXT] Re: [PATCH] usb: typec: tcpci: allow drp toggling for non-drp port
>>
>> Hi Guenter,
>>
>>> -----Original Message-----
>>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>>> Sent: Thursday, June 8, 2023 9:24 PM
>>> To: Xu Yang <xu.yang_2@nxp.com>; heikki.krogerus@linux.intel.com
>>> Cc: gregkh@linuxfoundation.org; dl-linux-imx <linux-imx@nxp.com>; linux-usb@vger.kernel.org; Jun Li
>> <jun.li@nxp.com>;
>>> Zhipeng Wang <zhipeng.wang_1@nxp.com>; Faqiang Zhu <faqiang.zhu@nxp.com>
>>> Subject: [EXT] Re: [PATCH] usb: typec: tcpci: allow drp toggling for non-drp port
>>>
>>> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the
>>> message using the 'Report this email' button
>>>
>>>
>>> On 6/8/23 04:28, Xu Yang wrote:
>>>> Some single power role Type-C port with dual data role, this kind of
>>>> port connects non Type-C port for usb data will need tcpm to work to
>>>> get polarity for orientation change and let Type-C port keep at fake
>>>> power role to provide another non-default data role, so remove the drp
>>>> port condition for now.
>>>>
>>>> Has anyone encountered this use case? How do you handle this limitation
>>>> in current tcpm with a better way? Please kindly share your thoughts.
>>>>
>>>
>>> Have you ? This is an odd comment to make in the patch description.
>>
>> Sorry for this. It's not a formal patch.
>>
>>>
>>> Either case, I don't understand why one would need to enable toggling
>>> under any circumstances if the port is not DRP. The description does
>>> not explain how "need tcpm to work" correlates to enabling toggling on
>>> non-DRP ports.
>>
>> My case is a source-only PD capable port with dual data role, connect to
>> legacy PC host via A-to-C to work as USB device mode. Under current tcpm
>> mechanism, the PC will not recognize the source-only PD capable port and
>> the usb controller has no chance to work as device mode.
>>
>> However, if I enable CC toggling, the PD port can re-work as sink role and
>> the USB controller can function as device mode too. Since it's USB3 port,
>> it can work only after the SS link has correctly established. Thus, we also
>> need tcpm to set correct orientation.
>>
>> So, it seems a limitation to let single power role Type-C port with dual
>> data role to work as non-default data role when connected to non Type-C
>> port.
> 
> I do see below patches from Hans to fix the same issue as ours.
> 
> 48242e30532b ("usb: typec: fusb302: Revert "Resolve fixed power role contract setup"")
> 6258db14d78c ("usb: typec: fusb302: Implement start_toggling for all port-types")
> 7893f9e1c26d ("usb: typec: tcpm: Notify the tcpc to start connection-detection for SRPs")
> 
> So I thinks it's really a limitation for SRP to work correctly with tcpci driver.
> Maybe an improvement is needed for tcpci driver.

Thank you for Cc-ing me. This situation is a bit different from the one fixed by the above patches.

The above patches where for pure single-role ports.

Where as what we seem to have here is a dual-data-role, single-power-role port.

I assume that for this port at least the 5v boost output can be turned on/off. But I guess that it cannot be used to charge the device ?

To me it sounds like that to make this work, even with dumb (just a resistor on 1 Cc line) USB-C - USB-A cables, the port should simply be declared as a dual-role port, because that is wat it actually is (it actually is dual-role).

And then when configured as sink, it can operate in the default device-mode sink data role and just not consume the provided 5V.

Note if the 5V boost output can not be disabled that that would be a problem but that would really be out of spec, you cannot just unconditionally output 5V from a Type-C port.

Regards,

Hans



>>>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
>>>> Signed-off-by: Li Jun <jun.li@nxp.com>
>>>> ---
>>>>   drivers/usb/typec/tcpm/tcpci.c | 3 ---
>>>>   drivers/usb/typec/tcpm/tcpm.c  | 6 +++++-
>>>>   2 files changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
>>>> index fc708c289a73..88559e749120 100644
>>>> --- a/drivers/usb/typec/tcpm/tcpci.c
>>>> +++ b/drivers/usb/typec/tcpm/tcpci.c
>>>> @@ -175,9 +175,6 @@ static int tcpci_start_toggling(struct tcpc_dev *tcpc,
>>>>       struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>>>>       unsigned int reg = TCPC_ROLE_CTRL_DRP;
>>>>
>>>> -     if (port_type != TYPEC_PORT_DRP)
>>>> -             return -EOPNOTSUPP;
>>>> -
>>>>       /* Handle vendor drp toggling */
>>>>       if (tcpci->data->start_drp_toggling) {
>>>>               ret = tcpci->data->start_drp_toggling(tcpci, tcpci->data, cc);
>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>>>> index 3c6b0c8e2d3a..6aa62132e69a 100644
>>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>>> @@ -4274,7 +4274,11 @@ static void run_state_machine(struct tcpm_port *port)
>>>>               ret = tcpm_snk_attach(port);
>>>>               if (ret < 0)
>>>>                       tcpm_set_state(port, SNK_UNATTACHED, 0);
>>>> -             else
>>>> +             else if (port->port_type == TYPEC_PORT_SRC &&
>>>> +                      port->typec_caps.data == TYPEC_PORT_DRD) {
>>>> +                     tcpm_typec_connect(port);
>>>> +                     tcpm_log(port, "Keep at SNK_ATTACHED for USB data.");
>>>> +             } else
>>>>                       tcpm_set_state(port, SNK_STARTUP, 0);
>>>>               break;
>>>>       case SNK_STARTUP:
>