RE: [PATCH] staging: typec: handle vendor defined part and modify drp toggling flow

From: Jun Li
Date: Fri Mar 02 2018 - 09:39:03 EST

Next message: Corey Minyard: "Re: [PATCH] ipmi: use ARRAY_SIZE for poweroff_functions array sizing calculation"
Previous message: Jan Glauber: "[PATCH 2/2] arm64: defconfig: Raise NR_CPUS to 256"
In reply to: shufan_lee(ææå): "RE: [PATCH] staging: typec: handle vendor defined part and modify drp toggling flow"
Next in thread: ææå: "Re: [PATCH] staging: typec: handle vendor defined part and modify drp toggling flow"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi
> -----Original Message-----
> From: shufan_lee(ææå) [mailto:shufan_lee@xxxxxxxxxxx]
> Sent: 2018å3æ1æ 19:54
> To: Jun Li <jun.li@xxxxxxx>; ShuFanLee <leechu729@xxxxxxxxx>;
> heikki.krogerus@xxxxxxxxxxxxxxx; linux@xxxxxxxxxxxx; greg@xxxxxxxxx
> Cc: cy_huang(éåå) <cy_huang@xxxxxxxxxxx>;
> linux-kernel@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>
> Subject: RE: [PATCH] staging: typec: handle vendor defined part and modify
> drp toggling flow
>
> Hi Jun,
>
> > -----Original Message-----
> > From: Jun Li [mailto:jun.li@xxxxxxx]
> > Sent: Thursday, March 01, 2018 6:06 PM
> > To: shufan_lee(ææå); ShuFanLee; heikki.krogerus@xxxxxxxxxxxxxxx;
> > linux@xxxxxxxxxxxx; greg@xxxxxxxxx
> > Cc: cy_huang(éåå); linux-kernel@xxxxxxxxxxxxxxx;
> > linux-usb@xxxxxxxxxxxxxxx; 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@xxxxxxxxxxx]
> > Sent: 2018å3æ1æ 16:49
> > To: Jun Li <jun.li@xxxxxxx>; ShuFanLee <leechu729@xxxxxxxxx>;
> > heikki.krogerus@xxxxxxxxxxxxxxx; linux@xxxxxxxxxxxx; greg@xxxxxxxxx
> > Cc: cy_huang(éåå) <cy_huang@xxxxxxxxxxx>;
> > linux-kernel@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; dl-linux-imx
> > <linux-imx@xxxxxxx>
> > 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

>
> [ 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?
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?
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)

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

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

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

Next message: Corey Minyard: "Re: [PATCH] ipmi: use ARRAY_SIZE for poweroff_functions array sizing calculation"
Previous message: Jan Glauber: "[PATCH 2/2] arm64: defconfig: Raise NR_CPUS to 256"
In reply to: shufan_lee(ææå): "RE: [PATCH] staging: typec: handle vendor defined part and modify drp toggling flow"
Next in thread: ææå: "Re: [PATCH] staging: typec: handle vendor defined part and modify drp toggling flow"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]