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.
Thanks,
Xu Yang
>
> 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:
> >
