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 李書帆 <leechu...@gmail.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...@nxp.com>:
>>
>> Hi
>> > -Original Message-
>> > From: shufan_lee(李書帆) [mailto:shufan_...@richtek.com]
>> > Sent: 2018年3月1日 19:54
>> > To: Jun Li <jun...@nxp.com>; ShuFanLee <leechu...@gmail.com>;
>> > heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com
>> > Cc: cy_huang(黃啟原) <cy_hu...@richtek.com>;
>> > linux-ker...@vger.kernel.org; linux-usb@vger.kernel.org; dl-linux-imx
>> > <linux-...@nxp.com>
>> > Subject: RE: [PATCH] staging: typec: handle vendor defined part and modify
>> > drp toggling flow
>> >
>> > Hi Jun,
>> >
>> > > -Original Message-
>> > > From: Jun Li [mailto:jun...@nxp.com]
>> > > Sent: Thursday, March 01, 2018 6:06 PM
>> > > To: shufan_lee(李書帆); ShuFanLee; heikki.kroge...@linux.intel.com;
>> > > li...@roeck-us.net; g...@kroah.com
>> > > Cc: cy_huang(黃啟原); linux-ker...@vger.kernel.org;
>> > > linux-usb@vger.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_...@richtek.com]
>> > > Sent: 2018年3月1日 16:49
>> > > To: Jun Li <jun...@nxp.com>; ShuFanLee <leechu...@gmail.com>;
>> > > heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com
>> > > Cc: cy_huang(黃啟原) <cy_hu...@richtek.com>;
>> > > linux-ker...@vger.kernel.org; linux-usb@vger.kernel.org; dl-linux-imx
>> > > <linux-...@nxp.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/x(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

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

[Jun Li]

Hi
> -Original Message-
> From: shufan_lee(李書帆) [mailto:shufan_...@richtek.com]
> Sent: 2018年3月1日 19:54
> To: Jun Li <jun...@nxp.com>; ShuFanLee <leechu...@gmail.com>;
> heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com
> Cc: cy_huang(黃啟原) <cy_hu...@richtek.com>;
> linux-ker...@vger.kernel.org; linux-usb@vger.kernel.org; dl-linux-imx
> <linux-...@nxp.com>
> Subject: RE: [PATCH] staging: typec: handle vendor defined part and modify
> drp toggling flow
> 
> Hi Jun,
> 
> > -Original Message-
> > From: Jun Li [mailto:jun...@nxp.com]
> > Sent: Thursday, March 01, 2018 6:06 PM
> > To: shufan_lee(李書帆); ShuFanLee; heikki.kroge...@linux.intel.com;
> > li...@roeck-us.net; g...@kroah.com
> > Cc: cy_huang(黃啟原); linux-ker...@vger.kernel.org;
> > linux-usb@vger.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_...@richtek.com]
> > Sent: 2018年3月1日 16:49
> > To: Jun Li <jun...@nxp.com>; ShuFanLee <leechu...@gmail.com>;
> > heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com
> > Cc: cy_huang(黃啟原) <cy_hu...@richtek.com>;
> > linux-ker...@vger.kernel.org; linux-usb@vger.kernel.org; dl-linux-imx
> > <linux-...@nxp.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/x(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.0

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

[李書帆]

Hi Jun,

> -Original Message-
> From: Jun Li [mailto:jun...@nxp.com]
> Sent: Thursday, March 01, 2018 6:06 PM
> To: shufan_lee(李書帆); ShuFanLee; heikki.kroge...@linux.intel.com; 
> li...@roeck-us.net; g...@kroah.com
> Cc: cy_huang(黃啟原); linux-ker...@vger.kernel.org; linux-usb@vger.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_...@richtek.com]
> Sent: 2018年3月1日 16:49
> To: Jun Li <jun...@nxp.com>; ShuFanLee <leechu...@gmail.com>;
> heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com
> Cc: cy_huang(黃啟原) <cy_hu...@richtek.com>;
> linux-ker...@vger.kernel.org; linux-usb@vger.kernel.org; dl-linux-imx
> <linux-...@nxp.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/x(your tcpc i2c device)
>
After applying your patch[2], TCPM log is as following:

[   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]

If TCPM does not enter SRC_ATTACHED state, RC.DRP will not be cleared.
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:

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

[Jun Li]

Hi Shufan

Please don't top posting

> -Original Message-
> From: shufan_lee(李書帆) [mailto:shufan_...@richtek.com]
> Sent: 2018年3月1日 16:49
> To: Jun Li <jun...@nxp.com>; ShuFanLee <leechu...@gmail.com>;
> heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com
> Cc: cy_huang(黃啟原) <cy_hu...@richtek.com>;
> linux-ker...@vger.kernel.org; linux-usb@vger.kernel.org; dl-linux-imx
> <linux-...@nxp.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/x(your tcpc i2c device)

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

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://www.spinics.net/lists/devicetree/msg216852.html

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_C

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

[Jun Li]

  
> ret = devm_request_threaded_irq(chip->dev, client->irq, NULL,
>   _tcpci_irq,
>   IRQF_ONESHOT |
> IRQF_TRIGGER_LOW,
>   dev_name(>dev),
> chip); }
> 
> 
Get it, thanks.

> Best Regards,
> *
> Shu-Fan Lee
> Richtek Technology Corporation
> TEL: +886-3-5526789 #2359
> FAX: +886-3-5526612
> *************
> 
> ____________________
> 寄件者: Jun Li <jun...@nxp.com>
> 寄件日期: 2018年2月22日 下午 06:16
> 收件者: ShuFanLee; heikki.kroge...@linux.intel.com; li...@roeck-us.net;
> g...@kroah.com
> 副本: shufan_lee(李��帆); cy_huang(�S�⒃�); linux-ker...@vger.kernel.org;
> linux-usb@vger.kernel.org; dl-linux-imx
> 主旨: RE: [PATCH] staging: typec: handle vendor defined part and modify drp
> toggling flow
> 
> Hi,
> 
> > -Original Message-
> > From: linux-usb-ow...@vger.kernel.org
> > [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of ShuFanLee
> > Sent: 2018年2月21日 23:02
> > To: heikki.kroge...@linux.intel.com; li...@roeck-us.net;
> > g...@kroah.com
> > Cc: shufan_...@richtek.com; cy_hu...@richtek.com;
> > linux-ker...@vger.kernel.org; linux-usb@vger.kernel.org
> > Subject: [PATCH] staging: typec: handle vendor defined part and modify
> > drp toggling flow
> >
> > From: ShuFanLee <shufan_...@richtek.com>
> >
> > Handle vendor defined behavior in tcpci_init, tcpci_set_vconn and
> > export tcpci_irq.
> > More operations can be extended in tcpci_data if needed.
> > According to TCPCI specification, 4.4.5.2 ROLE_CONTROL, TCPC shall not
> > start DRP toggling until subsequently the TCPM writes to the COMMAND
> > register to start DRP toggling.
> 
> My understanding of above statement is TCPM(this Linux driver) can enable
> DRP and CC1/CC2 in one shot, but TCPC(your typec chip internal firmware)
> needs wait until TCPM writes to COMMAND register to start drp toggling.
> 
> > DRP toggling flow is chagned as following:
> >   - Write DRP = 0 & Rd/Rd
> 
> Why fixed to be Rd/Rd? in this case, it means the starting value:
> 
> Tcpci 4.4.5.2:
> "When initiating autonomous DRP toggling, the TCPM shall write B6 (DRP)
> =1b and the starting value of Rp/Rd to B3..0 (CC1/CC2) to indicate DRP
> autonomous toggling mode to the TCPC."
> 
> >   - Write DRP = 1
> 
> What's your problem if combine above 2 in one shot?
> 
> >   - Set LOOK4CONNECTION command
> >
> > Signed-off-by: ShuFanLee <shufan_...@richtek.com>
> > ---
> >  drivers/staging/typec/tcpci.c | 128
> > +-
> >  drivers/staging/typec/tcpci.h |  13 +
> >  2 files changed, 115 insertions(+), 26 deletions(-)
> >
> >  patch changelogs between v1 & v2
> >  - Remove unnecessary i2c_client in the structure of tcpci
> >  - Rename structure of tcpci_vendor_data to tcpci_data
> >  - Not exporting tcpci read/write wrappers but register i2c regmap in
> > glue driver
> >  - Add set_vconn ops in tcpci_data
> >(It is necessary for RT1711H to enable/disable idle mode before
> > disabling/enabling vconn)
> >  - Export tcpci_irq so that vendor can call it in their own IRQ
> > handler
> >
> >  patch changelogs between v2 & v3
> >  - Change the return type of tcpci_irq from int to irqreturn_t
> >
> > diff --git a/drivers/staging/typec/tcpci.c
> > b/drivers/staging/typec/tcpci.c index 9bd4412..4959c69 100644
> > --- a/drivers/staging/typec/tcpci.c
> > +++ b/drivers/staging/typec/tcpci.c
> > @@ -21,7 +21,6 @@
> >
> >  struct tcpci {
> >   struct device *dev;
> > - struct i2c_client *client;
> >
> >   struct tcpm_port *port;
> >
> > @@ -30,6 +29,12 @@ struct tcpci {
> >   bool controls_vbus;
> >
> >   struct tcpc_dev tcpc;
> > + struct tcpci_data *data;
> > +};
> > +
> > +struct tcpci_chip {
> > + struct tcpci *tcpci;
> > + struct tcpci_data data;
> >  };
> >
> >  static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc) @@
> > -37,8
> > +42,7 @@ static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev
> > +*tcpc)
> >   return container_of(tcpc, struct tcpci, tcpc);  }
> >
> > -static int tcpci_read16(struct tcpci *tcpci, unsigned int reg,
> > - u16 *val)
> > +static int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16
> > +*val)
>

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

[Jun Li]

Hi,

> -Original Message-
> From: linux-usb-ow...@vger.kernel.org
> [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of ShuFanLee
> Sent: 2018年2月21日 23:02
> To: heikki.kroge...@linux.intel.com; li...@roeck-us.net; g...@kroah.com
> Cc: shufan_...@richtek.com; cy_hu...@richtek.com;
> linux-ker...@vger.kernel.org; linux-usb@vger.kernel.org
> Subject: [PATCH] staging: typec: handle vendor defined part and modify drp
> toggling flow
> 
> From: ShuFanLee 
> 
> Handle vendor defined behavior in tcpci_init, tcpci_set_vconn and export
> tcpci_irq.
> More operations can be extended in tcpci_data if needed.
> According to TCPCI specification, 4.4.5.2 ROLE_CONTROL, TCPC shall not
> start DRP toggling until subsequently the TCPM writes to the COMMAND
> register to start DRP toggling.

My understanding of above statement is TCPM(this Linux driver) can enable
DRP and CC1/CC2 in one shot, but TCPC(your typec chip internal firmware)
needs wait until TCPM writes to COMMAND register to start drp toggling.

> DRP toggling flow is chagned as following:
>   - Write DRP = 0 & Rd/Rd

Why fixed to be Rd/Rd? in this case, it means the starting value:

Tcpci 4.4.5.2:
"When initiating autonomous DRP toggling, the TCPM shall write B6 (DRP) =1b
and the starting value of Rp/Rd to B3..0 (CC1/CC2) to indicate DRP autonomous
toggling mode to the TCPC."

>   - Write DRP = 1

What's your problem if combine above 2 in one shot?

>   - Set LOOK4CONNECTION command
> 
> Signed-off-by: ShuFanLee 
> ---
>  drivers/staging/typec/tcpci.c | 128
> +-
>  drivers/staging/typec/tcpci.h |  13 +
>  2 files changed, 115 insertions(+), 26 deletions(-)
> 
>  patch changelogs between v1 & v2
>  - Remove unnecessary i2c_client in the structure of tcpci
>  - Rename structure of tcpci_vendor_data to tcpci_data
>  - Not exporting tcpci read/write wrappers but register i2c regmap in glue
> driver
>  - Add set_vconn ops in tcpci_data
>(It is necessary for RT1711H to enable/disable idle mode before
> disabling/enabling vconn)
>  - Export tcpci_irq so that vendor can call it in their own IRQ handler
> 
>  patch changelogs between v2 & v3
>  - Change the return type of tcpci_irq from int to irqreturn_t
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 9bd4412..4959c69 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -21,7 +21,6 @@
> 
>  struct tcpci {
>   struct device *dev;
> - struct i2c_client *client;
> 
>   struct tcpm_port *port;
> 
> @@ -30,6 +29,12 @@ struct tcpci {
>   bool controls_vbus;
> 
>   struct tcpc_dev tcpc;
> + struct tcpci_data *data;
> +};
> +
> +struct tcpci_chip {
> + struct tcpci *tcpci;
> + struct tcpci_data data;
>  };
> 
>  static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc) @@ -37,8
> +42,7 @@ static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
>   return container_of(tcpc, struct tcpci, tcpc);  }
> 
> -static int tcpci_read16(struct tcpci *tcpci, unsigned int reg,
> - u16 *val)
> +static int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16
> +*val)
>  {
>   return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16));  } @@
> -98,8 +102,10 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum
> typec_cc_status cc)  static int tcpci_start_drp_toggling(struct tcpc_dev
> *tcpc,
>   enum typec_cc_status cc)
>  {
> + int ret;
>   struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> - unsigned int reg = TCPC_ROLE_CTRL_DRP;
> + unsigned int reg = (TCPC_ROLE_CTRL_CC_RD <<
> TCPC_ROLE_CTRL_CC1_SHIFT) |
> +(TCPC_ROLE_CTRL_CC_RD <<
> TCPC_ROLE_CTRL_CC2_SHIFT);
> 
>   switch (cc) {
>   default:
> @@ -117,7 +123,19 @@ static int tcpci_start_drp_toggling(struct tcpc_dev
> *tcpc,
>   break;
>   }
> 
> - return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> + ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> + if (ret < 0)
> + return ret;
> + usleep_range(500, 1000);

Why need a wait here? It seems you actually don't want to do autonomously 
toggling
from the very beginning, instead, you begin with a directly control on CC, keep 
it(Rd)
for some time, then switch to use autonomously toggling, why you need such kind 
of
sequence? I think it's special and not following tcpci spec.
 
> + reg |= TCPC_ROLE_CTRL_DRP;
> + ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> + if (ret < 0)
> + return ret;
> + ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> +TCPC_CMD_LOOK4CONNECTION);
> + if (ret < 0)
> + return ret;
> + return 0;
>  }
> 
>  static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
> @@ -178,6 +196,16 @@ static int tcpci_set_vconn(struct tcpc_dev *tcpc,
> 

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

[Guenter Roeck]

On Wed, Feb 21, 2018 at 11:02:23PM +0800, ShuFanLee wrote:
> From: ShuFanLee 
> 
> Handle vendor defined behavior in tcpci_init, tcpci_set_vconn and export 
> tcpci_irq.
> More operations can be extended in tcpci_data if needed.
> According to TCPCI specification, 4.4.5.2 ROLE_CONTROL,
> TCPC shall not start DRP toggling until subsequently the TCPM
> writes to the COMMAND register to start DRP toggling.
> DRP toggling flow is chagned as following:
>   - Write DRP = 0 & Rd/Rd
>   - Write DRP = 1
>   - Set LOOK4CONNECTION command
> 
> Signed-off-by: ShuFanLee 

Mostly loooks good to me. Couple of nitpicks below.

Guenter

> ---
>  drivers/staging/typec/tcpci.c | 128 
> +-
>  drivers/staging/typec/tcpci.h |  13 +
>  2 files changed, 115 insertions(+), 26 deletions(-)
> 
>  patch changelogs between v1 & v2
>  - Remove unnecessary i2c_client in the structure of tcpci
>  - Rename structure of tcpci_vendor_data to tcpci_data
>  - Not exporting tcpci read/write wrappers but register i2c regmap in glue 
> driver
>  - Add set_vconn ops in tcpci_data
>(It is necessary for RT1711H to enable/disable idle mode before 
> disabling/enabling vconn)
>  - Export tcpci_irq so that vendor can call it in their own IRQ handler
> 
>  patch changelogs between v2 & v3
>  - Change the return type of tcpci_irq from int to irqreturn_t
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 9bd4412..4959c69 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -21,7 +21,6 @@
>  
>  struct tcpci {
>   struct device *dev;
> - struct i2c_client *client;
>  
>   struct tcpm_port *port;
>  
> @@ -30,6 +29,12 @@ struct tcpci {
>   bool controls_vbus;
>  
>   struct tcpc_dev tcpc;
> + struct tcpci_data *data;
> +};
> +
> +struct tcpci_chip {
> + struct tcpci *tcpci;
> + struct tcpci_data data;
>  };
>  
>  static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
> @@ -37,8 +42,7 @@ static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev 
> *tcpc)
>   return container_of(tcpc, struct tcpci, tcpc);
>  }
>  
> -static int tcpci_read16(struct tcpci *tcpci, unsigned int reg,
> - u16 *val)
> +static int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16 *val)
>  {
>   return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16));
>  }
> @@ -98,8 +102,10 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum 
> typec_cc_status cc)
>  static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
>   enum typec_cc_status cc)
>  {
> + int ret;
>   struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> - unsigned int reg = TCPC_ROLE_CTRL_DRP;
> + unsigned int reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
> +(TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
>  
>   switch (cc) {
>   default:
> @@ -117,7 +123,19 @@ static int tcpci_start_drp_toggling(struct tcpc_dev 
> *tcpc,
>   break;
>   }
>  
> - return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> + ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> + if (ret < 0)
> + return ret;
> + usleep_range(500, 1000);
> + reg |= TCPC_ROLE_CTRL_DRP;
> + ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> + if (ret < 0)
> + return ret;
> + ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> +TCPC_CMD_LOOK4CONNECTION);
> + if (ret < 0)
> + return ret;
> + return 0;

regmap_write returns a negative return code or 0, thus this can be
simplified to
return regmap_write(...);

>  }
>  
>  static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
> @@ -178,6 +196,16 @@ static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool 
> enable)
>   struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>   int ret;
>  
> + /* Handle vendor set vconn */
> + if (tcpci->data) {
> + if (tcpci->data->set_vconn) {
> + ret = tcpci->data->set_vconn(tcpci, tcpci->data,
> +  enable);
> + if (ret < 0)
> + return ret;
> + }
> + }
> +
>   ret = regmap_write(tcpci->regmap, TCPC_POWER_CTRL,
>  enable ? TCPC_POWER_CTRL_VCONN_ENABLE : 0);
>   if (ret < 0)
> @@ -323,6 +351,15 @@ static int tcpci_init(struct tcpc_dev *tcpc)
>   if (time_after(jiffies, timeout))
>   return -ETIMEDOUT;
>  
> + /* Handle vendor init */
> + if (tcpci->data) {
> + if (tcpci->data->init) {
> + ret = tcpci->data->init(tcpci, tcpci->data);
> + if (ret < 0)
> + return ret;
> + }
> + }
> +
>   /* Clear all 

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

[Heikki Krogerus]

On Wed, Feb 14, 2018 at 05:24:04PM +0800, ShuFanLee wrote:
> From: ShuFanLee 
> 
> Handle vendor defined behavior in tcpci_init and tcpci_irq.
> More operations can be extended in tcpci_vendor_data if needed.
> According to TCPCI specification, 4.4.5.2 ROLE_CONTROL,
> TCPC shall not start DRP toggling until subsequently the TCPM
> writes to the COMMAND register to start DRP toggling.
> DRP toggling flow is chagned as following:
>   - Write DRP = 0 & Rd/Rd
>   - Write DRP = 1
>   - Set LOOK4CONNECTION command
> 
> Signed-off-by: ShuFanLee 
> ---
>  drivers/staging/typec/tcpci.c | 98 
> ---
>  drivers/staging/typec/tcpci.h | 15 +++
>  2 files changed, 97 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 9bd4412..b3a97b3 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -30,6 +30,7 @@ struct tcpci {
>   bool controls_vbus;
>  
>   struct tcpc_dev tcpc;
> + struct tcpci_vendor_data *vdata;
>  };
>  
>  static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
> @@ -37,16 +38,29 @@ static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev 
> *tcpc)
>   return container_of(tcpc, struct tcpci, tcpc);
>  }
>  
> -static int tcpci_read16(struct tcpci *tcpci, unsigned int reg,
> - u16 *val)
> +int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16 *val)
>  {
>   return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16));
>  }
> +EXPORT_SYMBOL_GPL(tcpci_read16);
>  
> -static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
> +int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16 val)
>  {
>   return regmap_raw_write(tcpci->regmap, reg, , sizeof(u16));
>  }
> +EXPORT_SYMBOL_GPL(tcpci_write16);
> +
> +int tcpci_read8(struct tcpci *tcpci, unsigned int reg, u8 *val)
> +{
> + return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u8));
> +}
> +EXPORT_SYMBOL_GPL(tcpci_read8);
> +
> +int tcpci_write8(struct tcpci *tcpci, unsigned int reg, u8 val)
> +{
> + return regmap_raw_write(tcpci->regmap, reg, , sizeof(u8));
> +}
> +EXPORT_SYMBOL_GPL(tcpci_write8);

I don't think there is any need to export those wrappers. You can
always expect the glue driver to supply the regmap as member of that
struct tcpci_vendor_data. See below..

>  static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
>  {
> @@ -98,8 +112,10 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum 
> typec_cc_status cc)
>  static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
>   enum typec_cc_status cc)
>  {
> + int ret;
>   struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> - unsigned int reg = TCPC_ROLE_CTRL_DRP;
> + unsigned int reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
> +(TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
>  
>   switch (cc) {
>   default:
> @@ -116,8 +132,19 @@ static int tcpci_start_drp_toggling(struct tcpc_dev 
> *tcpc,
>   TCPC_ROLE_CTRL_RP_VAL_SHIFT);
>   break;
>   }
> -
> - return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> + ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> + if (ret < 0)
> + return ret;
> + usleep_range(500, 1000);
> + reg |= TCPC_ROLE_CTRL_DRP;
> + ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> + if (ret < 0)
> + return ret;
> + ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> +TCPC_CMD_LOOK4CONNECTION);
> + if (ret < 0)
> + return ret;
> + return 0;
>  }
>  
>  static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
> @@ -323,6 +350,15 @@ static int tcpci_init(struct tcpc_dev *tcpc)
>   if (time_after(jiffies, timeout))
>   return -ETIMEDOUT;
>  
> + /* Handle vendor init */
> + if (tcpci->vdata) {
> + if (tcpci->vdata->init) {
> + ret = (*tcpci->vdata->init)(tcpci, tcpci->vdata);

ret = tcpci->vdata->init(...

> + if (ret < 0)
> + return ret;
> + }
> + }
> +
>   /* Clear all events */
>   ret = tcpci_write16(tcpci, TCPC_ALERT, 0x);
>   if (ret < 0)
> @@ -351,6 +387,13 @@ static irqreturn_t tcpci_irq(int irq, void *dev_id)
>  
>   tcpci_read16(tcpci, TCPC_ALERT, );
>  
> + /* Handle vendor defined interrupt */
> + if (tcpci->vdata) {
> + if (tcpci->vdata->irq_handler)
> + (*tcpci->vdata->irq_handler)(tcpci, tcpci->vdata,
> +  );

Ditto.

> + }
> +
>   /*
>* Clear alert status for everything except RX_STATUS, which shouldn't
>* be cleared until we have successfully retrieved message.
> @@