Hi,
On 6/20/23 11:38, Xu Yang wrote:
> Hi Hans,
>
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@xxxxxxxxxx>
>> Sent: Tuesday, June 20, 2023 4:35 PM
>> To: Xu Yang <xu.yang_2@xxxxxxx>; Guenter Roeck <linux@xxxxxxxxxxxx>; heikki.krogerus@xxxxxxxxxxxxxxx
>> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; Jun Li <jun.li@xxxxxxx>;
>> Zhipeng Wang <zhipeng.wang_1@xxxxxxx>; Faqiang Zhu <faqiang.zhu@xxxxxxx>
>> Subject: Re: [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
>>
>>
>> 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@xxxxxxxxxxxx>; heikki.krogerus@xxxxxxxxxxxxxxx
>>>> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; Jun Li
>> <jun.li@xxxxxxx>;
>>>> Zhipeng Wang <zhipeng.wang_1@xxxxxxx>; Faqiang Zhu <faqiang.zhu@xxxxxxx>
>>>> Subject: RE: [EXT] Re: [PATCH] usb: typec: tcpci: allow drp toggling for non-drp port
>>>>
>>>> Hi Guenter,
>>>>
>>>>> -----Original Message-----
>>>>> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
>>>>> Sent: Thursday, June 8, 2023 9:24 PM
>>>>> To: Xu Yang <xu.yang_2@xxxxxxx>; heikki.krogerus@xxxxxxxxxxxxxxx
>>>>> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; Jun Li
>>>> <jun.li@xxxxxxx>;
>>>>> Zhipeng Wang <zhipeng.wang_1@xxxxxxx>; Faqiang Zhu <faqiang.zhu@xxxxxxx>
>>>>> 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.
>
> Thanks for your comments.
>
> I looked through your patches in detail. It seems that your issue is the
> TOGDONE interrupt cannot be asserted even the default CC termination is
> set for SRP when Type-C cable attached. The reason is a step to enable
> SNK or SRC Toggle autonomous functionality is missed. When this step is
> added, TOGDONE interrupt will come as expected. However, it has issue to
> put this step to tcpm_set_cc() when doing power role swap. So your case
> should be a different issue from mine.
>
> Yes, the 5V output from source-only port can be turned on/off. The tcpc
> does support dual power role. But subject to our usecase, we need to
> restrict it to be a source-only port with below properties and avoid sinking
> vbus via hw design:
>
> power-role = "source";
> data-role = "dual";
>
> So I think this is a normal usecase. The software might need to make a
> little change to support it.
Right, but in order to be able to use device-mode with a passive
USB-C <-> USB-A cable your tcpc still needs to use dual-role toggling,
otherwise it will not even generate an IRQ when plugging in the
passive USB-C <-> USB-A cable and you thus will not even get
an insertion event.
Which I see is more or less what your original patch tries to do.
So I guess your original patch does seem to be something like
what is necessary but for the UCSI bit it should be limited to
only still allow dual-role toggling when the data-role is "dual".
Also it should be split into separate patches for the UCSI
and TCPM parts.
Note I've not looked at the TCPM part of the patch in detail,
I'm not sure that part is correct.
Regards,
Hans
>>>>>> Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
>>>>>> Signed-off-by: Li Jun <jun.li@xxxxxxx>
>>>>>> ---
>>>>>> 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:
>>>
>
