From: Hans de Goede <hdegoede@redhat.com> To: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>, Kyle Tso <kyletso@google.com> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>, Guenter Roeck <linux@roeck-us.net>, 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> Subject: Re: [PATCH v2] usb: typec: tcpm: collision avoidance Date: Sat, 13 Apr 2019 22:38:49 +0200 [thread overview] Message-ID: <009662c6-2897-e2dd-03a7-992fc0a78599@redhat.com> (raw) In-Reply-To: <76a3c6df-63c0-78e7-c1ca-c83a30e95d38@redhat.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Hans de Goede <hdegoede@redhat.com> To: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>, Kyle Tso <kyletso@google.com> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>, Guenter Roeck <linux@roeck-us.net>, 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> Subject: [v2] usb: typec: tcpm: collision avoidance Date: Sat, 13 Apr 2019 22:38:49 +0200 [thread overview] Message-ID: <009662c6-2897-e2dd-03a7-992fc0a78599@redhat.com> (raw) 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
next prev parent reply other threads:[~2019-04-13 20:39 UTC|newest] Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-03-22 12:17 [PATCH v2] usb: typec: tcpm: collision avoidance Kyle Tso 2019-03-22 12:17 ` [v2] " Kyle Tso 2019-04-04 14:13 ` [PATCH v2] " Heikki Krogerus 2019-04-04 14:13 ` [v2] " Heikki Krogerus 2019-04-05 13:42 ` [PATCH v2] " Guenter Roeck 2019-04-05 13:42 ` [v2] " Guenter Roeck 2019-04-08 14:17 ` [PATCH v2] " Kyle Tso 2019-04-08 14:17 ` [v2] " Kyle Tso 2019-04-09 13:02 ` [PATCH v2] " Heikki Krogerus 2019-04-09 13:02 ` [v2] " Heikki Krogerus 2019-04-09 13:06 ` [PATCH v2] " Heikki Krogerus 2019-04-09 13:06 ` [v2] " Heikki Krogerus 2019-04-09 14:41 ` [PATCH v2] " Hans de Goede 2019-04-09 14:41 ` [v2] " Hans de Goede 2019-04-10 10:32 ` [PATCH v2] " Adam Thomson 2019-04-10 10:32 ` [v2] " Opensource [Adam Thomson] 2019-04-10 12:49 ` [PATCH v2] " Kyle Tso 2019-04-10 12:49 ` [v2] " Kyle Tso 2019-04-10 15:45 ` [PATCH v2] " Hans de Goede 2019-04-10 15:45 ` [v2] " Hans de Goede 2019-04-10 16:14 ` [PATCH v2] " Adam Thomson 2019-04-10 16:14 ` [v2] " Opensource [Adam Thomson] 2019-04-10 16:38 ` [PATCH v2] " Hans de Goede 2019-04-10 16:38 ` [v2] " Hans de Goede 2019-04-10 16:49 ` [PATCH v2] " Hans de Goede 2019-04-10 16:49 ` [v2] " Hans de Goede 2019-04-11 9:06 ` [PATCH v2] " Adam Thomson 2019-04-11 9:06 ` [v2] " Opensource [Adam Thomson] 2019-04-13 20:38 ` Hans de Goede [this message] 2019-04-13 20:38 ` Hans de Goede 2019-04-15 10:03 ` [PATCH v2] " Adam Thomson 2019-04-15 10:03 ` [v2] " Opensource [Adam Thomson] 2019-09-19 10:48 ` [PATCH v2] " Kyle Tso 2019-09-19 11:00 ` Heikki Krogerus 2019-09-19 13:31 ` Guenter Roeck
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=009662c6-2897-e2dd-03a7-992fc0a78599@redhat.com \ --to=hdegoede@redhat.com \ --cc=Adam.Thomson.Opensource@diasemi.com \ --cc=badhri@google.com \ --cc=gregkh@linuxfoundation.org \ --cc=heikki.krogerus@linux.intel.com \ --cc=kyletso@google.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=linux@roeck-us.net \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.