Message-ID: <AM5PR1001MB0994297457CC7E971C59C28B802F0@AM5PR1001MB0994.EURPRD10.PROD.OUTLOOK.COM>
Date:   Thu, 11 Apr 2019 09:06:52 +0000
From:   Adam Thomson <Adam.Thomson.Opensource@...semi.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        Adam Thomson <Adam.Thomson.Opensource@...semi.com>,
        Kyle Tso <kyletso@...gle.com>
CC:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Greg KH <gregkh@...uxfoundation.org>,
        Badhri Jagan Sridharan <badhri@...gle.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] usb: typec: tcpm: collision avoidance

On 10 April 2019 17:39, Hans de Goede wrote:

> 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@...semi.com> 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@...ck-us.net>
> >> 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=40326e857c57a0095d3f9d72c14cb13aef4ca56
> >>>>> 4
> >>>>>
> >>>>> 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=40326e857c57a0095d3f9d72c14cb13aef4ca56
> >>>>> 4
> >>>>>
> >>>>> 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.

Ok, it's maybe toggling in a slightly different sense as you're not toggling
roles but rather which CC pin you measure against. With regards to Rp being
toggled between CC1 and CC2, I don't actually see that mentioned in the
datasheet. Might be missing something though.

> 
> > 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.

Yes, ok that's true although originally almost all calls to tcpm_set_cc() seemed
to relate to handling the detached -> attached state. That obviously now would
change with Kyle's updates. However I do also see the PR Swap called into this
function so may have been affected (your tests will verify that) although for
the state machine that again then drops into a detached state. If we do add
a generic function for this I'd suggest maybe a different name as I don't think
toggling sounds right here. Might be a bit confusing given that toggling was
originally used in the scope of swapping roles.

> 
> > 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.

Ok, that's fine. Thanks.

> 
> Regards,
> 
> Hans

Powered by blists - more mailing lists