Re: [PATCH v2] usb: typec: tcpm: collision avoidance
From: Hans de Goede
Date: Wed Apr 10 2019 - 12:39:03 EST
Hi,
On 10-04-19 18:14, Adam Thomson wrote:
On 10 April 2019 16:45, Hans de Goede wrote:
Hi Kyle,
On 10-04-19 14:49, Kyle Tso wrote:
On Wed, Apr 10, 2019 at 6:32 PM Adam Thomson
<Adam.Thomson.Opensource@xxxxxxxxxxx> wrote:
On 09 April 2019 15:41, Hans de Goede wrote:
Hi,
On 09-04-19 15:06, Heikki Krogerus wrote:
On Tue, Apr 09, 2019 at 04:02:30PM +0300, Heikki Krogerus wrote:
+Hans
On Mon, Apr 08, 2019 at 10:17:35PM +0800, Kyle Tso wrote:
On Fri, Apr 5, 2019 at 9:42 PM Guenter Roeck <linux@xxxxxxxxxxxx>
wrote:
On 4/4/19 7:13 AM, Heikki Krogerus wrote:
Hi,
On Fri, Mar 22, 2019 at 08:17:45PM +0800, Kyle Tso wrote:
This patch provides the implementation of Collision Avoidance
introduced in PD3.0. The start of each Atomic Message Sequence
(AMS) initiated by the port will be denied if the current AMS
is not interruptible. The Source port will set the CC to
SinkTxNG if it is going to initiate an AMS, and SinkTxOk otherwise.
Meanwhile, any AMS initiated by a Sink port will be denied in
TCPM if the port partner (Source) sets SinkTxNG except for
HARD_RESET
and SOFT_RESET.
I tested this with my GDBWin which has fusb302. When I plug-in
DisplayPort adapter, the partner device never gets registered,
and I see steady flow of warnings from fusb302:
FWIW, I made multiple attempts to review the patch. Each time I
get stuck after a while and notice that I don't understand what is going
on.
Maybe the state machine needs a complete overhaul. It seems to
have reached a point where it is getting too complex to
understand what is going
on.
[ 693.391176] Vconn is on during toggle start [ 693.391250]
WARNING: CPU: 2 PID: 30 at drivers/usb/typec/tcpm/fusb302.c:562
fusb302_set_toggling+0x129/0x130 [fusb302] [ 693.400293]
Modules
linked in: intel_xhci_usb_role_switch fusb302 tcpm roles pi3usb30532
i915 typec intel_gtt intel_cht_int33fe
[ 693.406309] CPU: 2 PID: 30 Comm: kworker/u8:1 Tainted: G W
5.1.0-rc3-heikki+ #17
[ 693.408434] cht_wcove_pwrsrc cht_wcove_pwrsrc: Could not
detect charger type [ 693.412278] Hardware name: Default
string Default string/Default string, BIOS 5.11 05/25/2017 [
693.412283]
Workqueue: i2c-fusb302 tcpm_state_machine_work [tcpm] [
693.424256] RIP: 0010:fusb302_set_toggling+0x129/0x130
[fusb302] [ 693.427234] Code: 89 df e8 da ef ff ff 85 c0 78 c6
c6 83 b0 01 00
00 00 eb b7 b9 02 00 00 00 e9 48 ff ff ff 48 c7 c7 20 e8 21 a0
e8 8e 0c e4 e0 <0f> 0b e9 58 ff ff ff 41 55 4c 8d 6f e8 41 54
41 89
f4 55 53 48 8d [ 693.436204] RSP: 0000:ffffc9000076bd90 EFLAGS:
00010286 [ 693.439174] RAX: 0000000000000000 RBX:
ffff888178080028 RCX: 0000000000000000 [ 693.442157] RDX:
000000000000001f RSI: ffffffff8259051f RDI: ffffffff8259091f [
693.445130] RBP: 0000000000000003 R08: ffffffff82590500 R09:
00000000000202c0 [ 693.448100] R10: 0000010cb24a3d18 R11:
000000000000001e R12: ffff8881780801b0 [ 693.451086] R13:
ffffffffa021e4e5 R14: 0000000000000003 R15: ffff888178080040 [ 693.454060]
FS:
0000000000000000(0000) GS:ffff88817bb00000(0000)
knlGS:0000000000000000 [ 693.460009] CS: 0010 DS: 0000 ES: 0000 CR0:
0000000080050033 [ 693.462984] CR2:
00000000f7fb74a0 CR3: 000000000200d000 CR4: 00000000001006e0 [
693.465969] Call Trace:
[ 693.468937] tcpm_set_cc+0xb9/0x170 [fusb302] [ 693.471894]
tcpm_ams_start+0x1b8/0x2a0 [tcpm]
tcpm_ams_start() sets TYPEC_CC_RP_1_5 unconditionally, no matter
what. This causes the fusb302 code to start toggling. As such,
it may well attempt to start toggling in the wrong state.
Guenter
I read the fusb302 spec but failed to find the statement that
says it should "set toggling" when CC switches among
default/medium/high.
quot from fusb302 spec:
"The FUSB302 allows the host software to change the charging
current capabilities of the port through the HOST_CUR control
bits. If the HOST_CUR bits are changed prior to attach, the
FUSB302 automatically indicates the programmed current capability
when a device is attached.
If the current capabilities are changed after a device is
attached, the FUSB302 immediately changes the CC line to the
programmed capability."
Is it possible to skip fusb302_set_toggling() @ line#658 if
tcpm_set_cc() is called in order to switch the cc among
default/medium/high of Rp ?
Hans, you introduced that in commit daf81d0137a9c ("usb: typec:
fusb302: Refactor / simplify tcpm_set_cc()"), so could you take a
look at this.
I do not believe that that commit introduces the
fusb302_set_toggling() as the subject of the commit says it just
refactors things, the set_toggling call was introduced by:
commit ea3b4d5523bc8("usb: typec: fusb302: Resolve fixed power role
contract
setup")
Before that:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/t
ree/drivers/u
sb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca564
tcpm_set_cc actually turned toggling off in all cases.
I've no doubt that Adam was seeing a real problem, but I've doubted
if this was the right fix before. I even had it reverted in my tree
for a while, but since in my use-cases so far it has not caused any problems
I've not looked into it further.
From my recollection, that was the only way to generate the
necessary event from
fusb302 to indicate a connection, when the device was in a fixed role
state (i.e. only source or only sink). Without it the driver doesn't
work in these scenarios as there's no TOGDONE event generated by
fusb302, so no eventual call to 'tcpm_cc_change()' to tell TCPM that
something has happened and move on the state machine. Not all devices will
be DRP so we have to account for this.
The switch among different Rp values on CC pins comes from TCPM and
after the switch finishes, TCPM doesn't need to update the CC status
because this kind of switch won't affect the state machine.
In the mean time the code has changed quite a bit though, so making
tcpm_set_cc() behave as it did before, see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/t
ree/drivers/u
sb/typec/tcpm/fusb302.c?id=40326e857c57a0095d3f9d72c14cb13aef4ca564
Will require writing something from scratch based on the new code
which mimicks the behaviour of the old code; and then we also need
to fix Adam's problem on top.
Regards,
Hans
I tried to fix this with below changes and it works.
============================================
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -110,6 +110,9 @@ struct fusb302_chip {
enum typec_cc_status cc2;
u32 snk_pdo[PDO_MAX_OBJECTS];
+ /* Local pin status */
+ enum typec_cc_status cc;
+
#ifdef CONFIG_DEBUG_FS
struct dentry *dentry;
/* lock for log buffer access */ @@ -611,6 +614,19 @@ static
int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
enum toggling_mode mode;
mutex_lock(&chip->lock);
+ if ((chip->cc == TYPEC_CC_RP_DEF || chip->cc == TYPEC_CC_RP_1_5 ||
+ chip->cc == TYPEC_CC_RP_3_0) && (cc == TYPEC_CC_RP_DEF ||
+ cc == TYPEC_CC_RP_1_5 || cc == TYPEC_CC_RP_3_0)) {
+ ret = fusb302_set_src_current(chip, cc_src_current[cc]);
+ if (ret < 0) {
+ fusb302_log(chip, "cannot set src current %s, ret=%d\n",
+ typec_cc_status_name[cc], ret);
+ goto done;
+ }
+ fusb302_log(chip, "cc := %s", typec_cc_status_name[cc]);
+ goto rp_switch;
+ }
+
switch (cc) {
case TYPEC_CC_OPEN:
mode = TOGGLING_MODE_OFF; @@ -659,6 +675,8 @@ static
int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc)
if (ret < 0)
fusb302_log(chip, "cannot set toggling mode, ret=%d",
ret);
+rp_switch:
+ chip->cc = cc;
done:
mutex_unlock(&chip->lock);
I understand what you are trying to do here and I agree that just changing the Cc
pins this way should not start toggling. But I would rather go back to the
functionality of tcpm_set_cc() from before commit ea3b4d5523bc8("usb: typec:
fusb302: Resolve fixed power role contract setup")
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.
I'm also not 100%
convinced yet that this is something to resolve in TCPM as the reporting
mechanism is there to kick-on the TCPM state machine. It just needs the device
driver to know when to do it, hence the reason for my change.
Think maybe this needs a little more consideration before breaking something
to fix something else.
It is not my intention for the patch I plan to write to go
upstream as is, knowing that it will break your use-case.
Worst case we end up having 2 patches where your use-case
is broken in the intermediate state. But if we end up adding
a start_srp_toggling function as I suspect we may; then the
commit adding that may even be ordered before the other one,
so we can even avoid the broken intermediate state.
Regards,
Hans