Message-ID: <CAFaeLdTHY-OPsBecDNOjbn7fpZ8QVGVMJWiWttXoYZCNs6cFaw@mail.gmail.com>
Date:   Sat, 3 Mar 2018 02:43:16 +0800
From:   李書帆 <leechu729@...il.com>
To:     Jun Li <jun.li@....com>
Cc:     shufan_lee(李書帆) <shufan_lee@...htek.com>,
        "heikki.krogerus@...ux.intel.com" <heikki.krogerus@...ux.intel.com>,
        "linux@...ck-us.net" <linux@...ck-us.net>,
        "greg@...ah.com" <greg@...ah.com>,
        cy_huang(黃啟原) <cy_huang@...htek.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: Re: [PATCH] staging: typec: handle vendor defined part and modify drp
 toggling flow

Hi,

  Sorry, I modify it to plain text mode and send again

2018-03-03 2:39 GMT+08:00 李書帆 <leechu729@...il.com>:
>
> Hi Jun,
>
>   I think this operation should be moved to vendor's operation after rechecking the specification.
>
> 2018-03-02 22:38 GMT+08:00 Jun Li <jun.li@....com>:
>>
>> Hi
>> > -----Original Message-----
>> > From: shufan_lee(李書帆) [mailto:shufan_lee@...htek.com]
>> > Sent: 2018年3月1日 19:54
>> > To: Jun Li <jun.li@....com>; ShuFanLee <leechu729@...il.com>;
>> > heikki.krogerus@...ux.intel.com; linux@...ck-us.net; greg@...ah.com
>> > Cc: cy_huang(黃啟原) <cy_huang@...htek.com>;
>> > linux-kernel@...r.kernel.org; linux-usb@...r.kernel.org; dl-linux-imx
>> > <linux-imx@....com>
>> > Subject: RE: [PATCH] staging: typec: handle vendor defined part and modify
>> > drp toggling flow
>> >
>> > Hi Jun,
>> >
>> > > -----Original Message-----
>> > > From: Jun Li [mailto:jun.li@....com]
>> > > Sent: Thursday, March 01, 2018 6:06 PM
>> > > To: shufan_lee(李書帆); ShuFanLee; heikki.krogerus@...ux.intel.com;
>> > > linux@...ck-us.net; greg@...ah.com
>> > > Cc: cy_huang(黃啟原); linux-kernel@...r.kernel.org;
>> > > linux-usb@...r.kernel.org; dl-linux-imx
>> > > Subject: RE: [PATCH] staging: typec: handle vendor defined part and
>> > > modify drp toggling flow
>> > >
>> > > Hi Shufan
>> > >
>> > > Please don't top posting
>> > >
>> > > -----Original Message-----
>> > > From: shufan_lee(李書帆) [mailto:shufan_lee@...htek.com]
>> > > Sent: 2018年3月1日 16:49
>> > > To: Jun Li <jun.li@....com>; ShuFanLee <leechu729@...il.com>;
>> > > heikki.krogerus@...ux.intel.com; linux@...ck-us.net; greg@...ah.com
>> > > Cc: cy_huang(黃啟原) <cy_huang@...htek.com>;
>> > > linux-kernel@...r.kernel.org; linux-usb@...r.kernel.org; dl-linux-imx
>> > > <linux-imx@....com>
>> > > Subject: RE: [PATCH] staging: typec: handle vendor defined part and
>> > > modify drp toggling flow
>> > >
>> > > Hi Jun,
>> > >
>> > >   The attachment is waveform of the condition we met but I'm not sure
>> > > whether you can download the attachment.
>> > >   I add log in RT1711H it shows as following:
>> > >
>> > > You don't need add log by your own.
>> > > There is tcpm(./drivers/usb/typec/tcpm.c) log for debug already, which can
>> > show all the events and state transitions, you can get it by below command
>> > as I commented:
>> > >
>> > > cat /sys/kernel/debug/tcpm/xxxxx(your tcpc i2c device)
>> > >
>> > After applying your patch[2], TCPM log is as following:
>>
>> I assume you also applied my patch [1].
>> [1] https://www.spinics.net/lists/devicetree/msg216851.html
>>
> Yes, this patch is also applied.
>>
>> >
>> > [   53.050602] CC1: 0 -> 2, CC2: 0 -> 0 [state DRP_TOGGLING, polarity 0,
>> > connected]
>> > [   53.050613] state change DRP_TOGGLING -> SRC_ATTACH_WAIT
>> > [   53.050678] pending state change SRC_ATTACH_WAIT -> SNK_TRY @
>> > 200 ms
>> > => Rd is plugged out
>> > [   53.178804] CC1: 2 -> 0, CC2: 0 -> 0 [state SRC_ATTACH_WAIT, polarity 0,
>> > disconnected]
>> > [   53.178815] state change SRC_ATTACH_WAIT -> SRC_UNATTACHED
>> > => Rd is plugged in
>> > [   53.178874] Start DRP toggling
>> > [   53.188472] CC1: 0 -> 0, CC2: 0 -> 0 [state DRP_TOGGLING, polarity 0,
>> > disconnected]
>>
>> 1. The plug out and then plug in happens in 10ms? (53.188472 - 53.178804)
>> Was this done manually? Or by some special test method?
>
> It's done manually. If you can download the waveform attached in previous email, you can see Rd is plugged out/in within ms level.
> We connect a bridge board with test points of CC1, CC2 and GND to our platform's Typec receptacle. Then we can simulate a device plugging in/out by connecting/disconnecting a 5.1k resistor between CC1 and GND.
>
>>
>> 2. This is all tcpm log if you finally keep the Rd-device connected on typec
>> port? There is no more tcpm log after 53.188472 if you plug in Rd-device
>>
>> and don't remove it?
>
> Yes, RT1711H reports open/open to TCPM in drp_toggling state and stops toggling. Currently, TCPM does not restart drp_toggling if it receives open/open in drp_toggling state.
> I remember the specification of TypeC does not depict what TCPM should do if it receives open/open in drp_toggling state.
> Maybe restart toggling is an option.
>
>> 3. If the answer of Q2 is yes, then I must ask why you tcpc chip+internal firmware
>> can't report further cc change event after your drp toggling starts to present Rp(I know
>> it firstly present Rd after you write to LOOK4CONNECTION, but then it should change
>> to present Rp, so it should be able to detect the Rd-device finally)
>
> Because RT1711H's internal firmware changes to attached state when Rd is re-plugged in(cc level is default Rp (around 400mV) now).
> Then, LOOK4CONNECTION is set and RT1711H changes CCx to Rd that makes it reports open/open(because cc level changes from default Rp(around 400mV) to 0mV)
>>
>>
>> >
>> > If TCPM does not enter SRC_ATTACHED state, RC.DRP will not be cleared.
>>
>> In this case, you don’t need clear RC.DRP, see TCPCI spec:
>> "Figure 4-18. Sink Disconnect"
>> TCPM sink doesn't clear it in whole sequence, just directly set it:
>> Restart DRP Toggling
>> PC.AutoDischargeDisconnect=0b
>> Set RC.DRP=1b (DRP)
>> Set RC.CC1=10b (Rd)
>> Set RC.CC2=10b (Rd)
>> COMMAND.Look4Connection (DRP toggle)
>>
>>
>> > When TCPM writes Rd/Rd or Rp/Rp in the drp_toggling function, it does not
>> > take effect until LOOK4CONNECTION command is set.
>> > The above condition causes RT1711H reports open/open at [53.188472]
>> >
>> > > [ 914.937340] typec_rt1711h 2-004e: __rt1711h_irq_handler [
>> > > 914.943838] typec_rt1711h 2-004e: __tcpm_get_cc cc1 = Open, cc2 =
>> > Open
>> > > => Device(Rd) is plugged out
>> > >
>> > > [ 914.958041] typec_rt1711h 2-004e: tcpm_set_pd_rx 0 [ 914.963011]
>> > > typec_rt1711h 2-004e: tcpm_set_vbus vbus = 0 [ 914.968407]
>> > > typec_rt1711h
>> > > 2-004e: tcpm_set_vbus chg is already 0 [ 914.974541] typec_rt1711h
>> > 2-004e:
>> > > tcpm_set_vconn vconn is already 0 [ 914.980921] typec_rt1711h 2-004e:
>> > > tcpm_set_current_limit 0 ma, 0 mv (not implemented) [ 914.988894]
>> > > typec_rt1711h 2-004e: tcpm_set_polarity Polarity_CC1 [ 915.003201]
>> > > typec_rt1711h 2-004e: tcpm_set_roles Source Host [ 915.009264]
>> > > typec_rt1711h 2-004e: tcpm_start_drp_toggling => state_machine_work
>> > of
>> > > TCPM calls start_drp_toggling function but does not set
>> > > LOOK4CONNECTION command yet => (Note1) Device(Rd) is plugged in
>> > > (RT1711H's internal firmware detects Rd plugged in. cc_change is
>> > > triggered and it will be vRd-connected at this moment) => TCPM writes
>> > > LOOK4CONNECTION command
>> > > - Because RC.DRP is still 1, RT1711H will not pull cc1/cc2 to Rd while
>> > > writing Rd/Rd to RC.CC1/RC.CC2.
>> > > - (Note2) Right after LOOK4CONNECTION command is written, RT1711H
>> > > pulls CC to Rd's level (because RC.Role is Rd/Rd), stop toggling
>> > > (because
>> > > device(Rd) is already connected) and CC status will be open/open now
>> > > (because RT1711H presents Rd and device is connected(Rd))
>> > >
>> > > [ 915.037263] typec_rt1711h 2-004e: __tcpm_get_cc cc1 = Open, cc2 =
>> > > Open => Enter RT1711H's irq handler and it reports open/open
>> > >
>> > > I think the point is to write RC.DRP = 0 at the beginning of
>> > > drp_toggling so that RT1711H will pull cc1/cc2 to Rd while writing
>> > > Rd/Rd to RC.CC1/RC.CC2 This operation will make RT1711H's internal
>> > > firmware restarts from disconnected state and toggles correctly.
>> > >
>> > > According to TCPCI spec (4.4.5.2):
>> > > It is recommended the TCPM write ROLE_CONTROL.DRP=0 before writing
>> > to
>> > > POWER_CONTROL.AutoDischargeDisconnect and starting the DRP toggling
>> > > using COMMAND.Look4Connection Restart DRP Toggling => It is
>> > > recommended the TCPM write ROLE_CONTROL.DRP=0 here Set
>> > >
>> > > This statement is_not_ recommend you do this(RC.DRP=0) for start drp
>> > toggling, Please carefully check the whole sentence:
>> > > "... as shown in Figure 4-16, "
>> > > If you look at "Figure 4-16. DRP Initialization and Connection Detection"
>> > > It gives clear drp toggling start operations:
>> > >
>> > > Set TCPC to DRP
>> > > - Read PS.TCPCInitializationStatus
>> > > - Write ROLE_CONTROL
>> > > - RC.DRP = 1b
>> > > - RC.CC2=01b or 10b
>> > > - RC.CC1=01b or 10b
>> > > - RC.CC1=RC.CC2
>> > > - Write COMMAND.Look4ConnectionPS.
>> > >
>> > > Above is all operations required to start drp toggling. You also can see
>> > RC.CCx = 01b or 10b, not fixed to be Rd, right?
>> > Yes, this one should be like your patch[07/12]. Write Rd or Rp to RC.CCx
>> > according to the cc parameter of drp_toggling function.
>> > >
>> > > Go on to check the Figure 4-16
>> > > After debounce, we need do following:
>> > >
>> > > ConnectionDetermine CC & VCONN
>> > > - Write RC.CC1 & RC.CC2 per decision
>> > > - Write RC.DRP=0
>> > > - Write TCPC_CONTROl.PlugOrientation
>> > > - Write PC.AutoDischargeDisconnect=1
>> > >  & PC.EnableVconnConnection
>> > >
>> > > Current existing tcpm+tcpci will not clear RC.DRP after attach, That means
>> > RC.DRP clear may be done after attach, not in start_drp_toggling.
>> > > I am not sure if this can resolve your problem, but I think it deserve a try,
>> > you can follow above operation sequence while entering attach state, refer
>> > to my patch[2]:
>> > >
>> > > [2]
>> > >
>> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw
>> > ww
>> > > .spinics.net%2Flists%2Fdevicetree%2Fmsg216852.html&data=02%7C01%7
>> > Cjun.
>> > >
>> > li%40nxp.com%7C9117425550d24ddc86d808d57f6b1b4e%7C686ea1d3bc2b
>> > 4c6fa92c
>> > >
>> > d99c5c301635%7C0%7C0%7C636555020366483456&sdata=9%2BywYl%2BR
>> > PYtk60Wg6p
>> > > R63cCW2AnRXs%2BrINvvqUpqL18%3D&reserved=0
>> > >
>> > > diff --git a/drivers/staging/typec/tcpci.c
>> > > b/drivers/staging/typec/tcpci.c index 530a5d7..7145771 100644
>> > > --- a/drivers/staging/typec/tcpci.c
>> > > +++ b/drivers/staging/typec/tcpci.c
>> > > @@ -184,6 +184,7 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
>> > >                               enum typec_cc_polarity polarity)  {
>> > >         struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>> > > +       unsigned int reg;
>> > >         int ret;
>> > >
>> > >         ret = regmap_write(tcpci->regmap, TCPC_TCPC_CTRL, @@
>> > -192,6 +193,20 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
>> > >         if (ret < 0)
>> > >                 return ret;
>> > >
>> > > +       ret = regmap_read(tcpci->regmap, TCPC_ROLE_CTRL, &reg);
>> > > +       if (ret < 0)
>> > > +               return ret;
>> > > +
>> > > +       if (polarity == TYPEC_POLARITY_CC2)
>> > > +               ret = TCPC_ROLE_CTRL_CC1_SHIFT;
>> > > +       else
>> > > +               ret = TCPC_ROLE_CTRL_CC2_SHIFT;
>> > > +       reg |= TCPC_ROLE_CTRL_CC_OPEN << ret;
>> > > +       reg &= ~TCPC_ROLE_CTRL_DRP;
>> > > +       ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
>> > > +       if (ret < 0)
>> > > +               return ret;
>> > > +
>> > >         return 0;
>> > >  }
>> > >
>> > > PC.AutoDischargeDisconnect=0b Set RC.DRP=1b (DRP) Set RC.RpValue=00b
>> > > (smallest Rp to save power) Set RC.CC1=01b Set
>> > > RC.CC2=01bCOMMAND.Look4ConnectionNo
>> > >
>> > > It seems like it is not a general case here (because it is only
>> > > recommended but not necessary), we can move it to vendor_ops in the
>> > next patch.
>> > >
>> > > The TCPM should be able to cover all cases, and we should follow the
>> > recommended sequence(if what you are trying to do is really as spec says).
>> > Agree!
>> > My understanding is that we need to set RC.DRP to 0 every time before
>> > TCPM restarts toggling but not just for attached.
>>
>> Actually I have no objection on adding a RC.DRP clear, the question is
>> where is the right place to do this, till now I don’t see how this DRP bit keep set
>> while start drp toggling is causing problem, You want to start present Rd after write
>> CC1/CC2 to be Rd/Rd and before write to LOOK4CONNECTION, this is not required
>> per tcpci spec.
>
> According to TCPCI's specification, it is recommended the TCPM write RC.DRP = 0 before changing AutoDischargeDisconnecd.
> Maybe, an interface for controlling AutoDischargeDisconnecd can be added. Then clear RC.DRP in the beginning of that interface.
>>
>>
>> -Behavior after your patch:
>> //begin with Open as RC.DRP = 1
>> RC.CCx=Rd & RC.DRP = 0; //Start present Rd
>> Wait 1ms; // Still Rd
>> RC.CCx=Rd & RC.DRP = 1; //Open?
>> Look4CONNECTION = 1; //DRP toggling continue preset Rd
>>
>> Is my above CC state description correct? Is this what you want?
>
> Correct. But the purpose of setting RC.CCx = Rd & RC.DRP = 0 is to make RT1711H's internal firmware leave attached state.
> Because RT1711H is still presenting Rp after the device is plugged out, it's internal firmware enters attached state when the device re-plugged in.
> When we write RC.CCx = Rd & RC.DRP = 0, RT1711H's internal firmware leaves attached state. So it will keep toggling from Rd to Rp but not report open/open when LOOK4CONNECTION is set.
> After rechecking the specification of TCPCI, I think TCPC's internal firmware should start toggling from detached state when LOOK4CONNECTION is set even if RC.DRP is not cleared before
> I've discussed with my colleagues, we think this should be in vendor ops. I'll update it in the patch v6.
> Could I also apply your patch [1] in the patch v6?
> Thank you for your time to clarify this condition.
>>
>>
>> -Only apply my patches:
>> //begin with Open as RC.DRP = 1
>> RC.CCx=Rd & RC.DRP = 1; //still Open
>> Look4Connection = 1; //Start preset Rd
>>
>> > For RT1711H, it follows above flow. If it is not correct, this operation should
>> > be moved to vendor_ops.
>> > >
>> > > For your question:
>> > > Why RT1711H reports open/open after drp_toggling is enabled?
>> > > => See Note2 above.
>> > > This open/open is for you plug out the device, right?
>> > > => No, see Note2 above.
>> > > Why RT1711H can't report new cc change events after you plug in the
>> > > device?
>> > > => RT1711H can generate new cc change events after plugging in the
>> > device.
>> > > What cc change event tcpc will report on step 4?
>> > > => See Note1 above
>> > > Did you verify your change can pass the typec compliance test?
>> > > => We didn't test it yet but try to make all functions work correctly first.
>> > >
>> > > Best Regards,
>> > > *****************************
>> > > Shu-Fan Lee
>> > > Richtek Technology Corporation
>> > > TEL: +886-3-5526789 #2359
>> > > FAX: +886-3-5526612
>> > > *****************************
>> > >
>> >
>> > ************* Email Confidentiality Notice ********************
>> >
>> > The information contained in this e-mail message (including any attachments)
>> > may be confidential, proprietary, privileged, or otherwise exempt from
>> > disclosure under applicable laws. It is intended to be conveyed only to the
>> > designated recipient(s). Any use, dissemination, distribution, printing,
>> > retaining or copying of this e-mail (including its attachments) by unintended
>> > recipient(s) is strictly prohibited and may be unlawful. If you are not an
>> > intended recipient of this e-mail, or believe that you have received this
>> > e-mail in error, please notify the sender immediately (by replying to this
>> > e-mail), delete any and all copies of this e-mail (including any attachments)
>> > from your system, and do not disclose the content of this e-mail to any other
>> > person. Thank you!
>
>
>
>
> --
> Best Regards,
> 書帆




-- 
Best Regards,
書帆

Powered by blists - more mailing lists