On Tue, Sep 15, 2020 at 11:07:18AM +0800, ChiYuan Huang wrote: > Hi, Guenter: > > ChiYuan Huang <u0084500@xxxxxxxxx> 於 2020年9月6日 週日 下午11:22寫道： > > > > Guenter Roeck <linux@xxxxxxxxxxxx> 於 2020年9月5日 週六 下午11:51寫道： > > > > > > On 9/4/20 6:24 PM, ChiYuan Huang wrote: > > > > Guenter Roeck <linux@xxxxxxxxxxxx> 於 2020年9月5日 週六 上午3:41寫道： > > > >> > > > >> On 9/3/20 9:21 AM, ChiYuan Huang wrote: > > > >>> Guenter Roeck <linux@xxxxxxxxxxxx> 於 2020年9月3日 週四 上午12:57寫道： > > > >>>> > > > >>>> On Wed, Sep 02, 2020 at 11:35:33PM +0800, cy_huang wrote: > > > >>>>> From: ChiYuan Huang <cy_huang@xxxxxxxxxxx> > > > >>>>> > > > >>>>> Fix: If vbus event is before cc_event trigger, hard_reset_count > > > >>>>> won't bt reset for some case. > > > >>>>> > > > >>>>> Signed-off-by: ChiYuan Huang <cy_huang@xxxxxxxxxxx> > > > >>>>> --- > > > >>>>> Below's the flow. > > > >>>>> > > > >>>>> _tcpm_pd_vbus_off() -> run_state_machine to change state to SNK_UNATTACHED > > > >>>>> call tcpm_snk_detach() -> tcpm_snk_detach() -> tcpm_detach() > > > >>>>> tcpm_port_is_disconnected() will be called. > > > >>>>> But port->attached is still true and port->cc1=open and port->cc2=open > > > >>>>> > > > >>>>> It cause tcpm_port_is_disconnected return false, then hard_reset_count won't be reset. > > > >>>>> After that, tcpm_reset_port() is called. > > > >>>>> port->attached become false. > > > >>>>> > > > >>>>> After that, cc now trigger cc_change event, the hard_reset_count will be kept. > > > >>>>> Even tcpm_detach will be called, due to port->attached is false, tcpm_detach() > > > >>>>> will directly return. > > > >>>>> > > > >>>>> CC_EVENT will only trigger drp toggling again. > > > >>>>> --- > > > >>>>> drivers/usb/typec/tcpm/tcpm.c | 3 +-- > > > >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) > > > >>>>> > > > >>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > > >>>>> index a48e3f90..5c73e1d 100644 > > > >>>>> --- a/drivers/usb/typec/tcpm/tcpm.c > > > >>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c > > > >>>>> @@ -2797,8 +2797,7 @@ static void tcpm_detach(struct tcpm_port *port) > > > >>>>> port->tcpc->set_bist_data(port->tcpc, false); > > > >>>>> } > > > >>>>> > > > >>>>> - if (tcpm_port_is_disconnected(port)) > > > >>>>> - port->hard_reset_count = 0; > > > >>>>> + port->hard_reset_count = 0; > > > >>>>> > > > >>>> > > > >>>> Doesn't that mean that the state machine will never enter > > > >>>> error recovery ? > > > >>>> > > > >>> I think it does't affect the error recovery. > > > >>> All error recovery seems to check pd_capable flag. > > > >>> > > > >>> >From my below case, it's A to C cable only. There is no USBPD contract > > > >>> will be estabilished. > > > >>> > > > >>> This case occurred following by the below test condition > > > >>> Cable -> A to C (default Rp bind to vbus) connected to PC. > > > >>> 1. first time plugged in the cable with PC > > > >>> It will make HARD_RESET_COUNT to be equal 2 > > > >>> 2. And then plug out. At that time HARD_RESET_COUNT is till 2. > > > >>> 3. next time plugged in again. > > > >>> Due to hard_reset_count is still 2 , after wait_cap_timeout, the state > > > >>> eventually changed to SNK_READY. > > > >>> But during the state transition, no hard_reset be sent. > > > >>> > > > >>> Defined in the USBPD policy engine, typec transition to USBPD, all > > > >>> variables must be reset included hard_reset_count. > > > >>> So it expected SNK must send hard_reset again. > > > >>> > > > >>> The original code defined hard_reset_count must be reset only when > > > >>> tcpm_port_is_disconnected. > > > >>> > > > >>> It doesn't make sense that it only occurred in some scenario. > > > >>> If tcpm_detach is called, hard_reset count must be reset also. > > > >>> > > > >> > > > >> If a hard reset fails, the state machine may cycle through states > > > >> HARD_RESET_SEND, HARD_RESET_START, SRC_HARD_RESET_VBUS_OFF, > > > >> SRC_HARD_RESET_VBUS_ON back to SRC_UNATTACHED. In this state, > > > >> tcpm_src_detach() and with it tcpm_detach() is called. The hard > > > >> reset counter is incremented in HARD_RESET_SEND. If tcpm_detach() > > > >> resets the counter, the state machine will keep cycling through hard > > > >> resets without ever entering the error recovery state. I am not > > > >> entirely sure where the counter should be reset, but tcpm_detach() > > > >> seems to be the wrong place. > > > > > > > > This case you specified means locally error occurred. > > > > > > It could be a local error (with the local hardware), or with the > > > remote partner not accepting the reset. We only know that an error > > > occurred. > > > > > > > It intended to re-run the state machine from typec to USBPD. > > > >>From my understanding, hard_reset_count to be reset is reasonable. > > > > > > > > The normal stare from the state transition you specified is > > > > HARD_RESET_SEND, HARD_RESET_START -> SRC_HARD_RESET_VBUS_OFF, > > > > SRC_HARD_RESET_VBUS_ON -> received VBUS_EVENT then go to SRC_STARTUP. > > > > > > > The operational word is "normal". Error recovery is expected to handle > > > situations which are not normal. > > > > Following by the USBPD 3.0 revision 1.2, section 8.3.3.24.1 > > The ErrorRecovery state is used to electronically disconnect Port > > Partner using the USB Type-C connector. > > And there's one sentence to be said "The ErrorRecovery staste shall > > map to USB Type-C Error Recovery state operations". > > I also read ErrorRecovery state in USB TYPE-C 1.3 spec. > > Section 4.5.2.2.2.1 ErrorRecovery state requirement listed the below text. > > The port shall not drive VBUS or VCONN, and shall present a > > high-impedance to ground (above > > zOPEN) on its CC1 and CC2 pins. > > Section 4.5.2.2.2.2 Exiting from the error recovery state > > I read the description. The roughly meaning is to change the state to > > Unattached(Src or Snk) after tErrorRecovery. > > > > Summary the above text. > > Reset HardResetCounter is ok in tcpm_detach. > > My patch is just to relax the counter reset conditions during tcpm_detach(). > > If not, it will check tcpm_port_is_disconnected(). > > And only two scenario, the hard reset count will be cleared to 0 at this case. > > 1) port not attached and cc1=open and cc2=open > > 2) port attached and either (polarity=cc1, cc1=open) or (polarity=cc2, cc2=open) > > > > I think this judgement is narrow in tcpm_detach case. > > > > > > > > I don't question the need to reset the counter. The only question > > > is where and when to reset it. > > > > > I re-check all tcpm code for hard reset counter about the increment and reset. > > They all meets USBPD spec. Only the detach case, I'm wondering why it > > need to add the check for tcpm_port_is_disconnected(). > > > Below's the real case log. > [ 4848.046358] VBUS off > [ 4848.046384] state change SNK_READY -> SNK_UNATTACHED > [ 4848.050908] Setting voltage/current limit 0 mV 0 mA > [ 4848.050936] polarity 0 > [ 4848.052593] Requesting mux state 0, usb-role 0, orientation 0 > [ 4848.053222] Start toggling > [ 4848.086500] state change SNK_UNATTACHED -> TOGGLING > [ 4848.089983] CC1: 0 -> 0, CC2: 3 -> 3 [state TOGGLING, polarity 0, connected] > [ 4848.089993] state change TOGGLING -> SNK_ATTACH_WAIT > [ 4848.090031] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms > [ 4848.141162] CC1: 0 -> 0, CC2: 3 -> 0 [state SNK_ATTACH_WAIT, > polarity 0, disconnected] > [ 4848.141170] state change SNK_ATTACH_WAIT -> SNK_ATTACH_WAIT > [ 4848.141184] pending state change SNK_ATTACH_WAIT -> SNK_UNATTACHED @ 20 ms > [ 4848.163156] state change SNK_ATTACH_WAIT -> SNK_UNATTACHED [delayed 20 ms] > [ 4848.163162] Start toggling > [ 4848.216918] CC1: 0 -> 0, CC2: 0 -> 3 [state TOGGLING, polarity 0, connected] > [ 4848.216954] state change TOGGLING -> SNK_ATTACH_WAIT > [ 4848.217080] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms > [ 4848.231771] CC1: 0 -> 0, CC2: 3 -> 0 [state SNK_ATTACH_WAIT, > polarity 0, disconnected] > [ 4848.231800] state change SNK_ATTACH_WAIT -> SNK_ATTACH_WAIT > [ 4848.231857] pending state change SNK_ATTACH_WAIT -> SNK_UNATTACHED @ 20 ms > [ 4848.256022] state change SNK_ATTACH_WAIT -> SNK_UNATTACHED [delayed 20 ms] > [ 4848.256049] Start toggling > [ 4848.871148] VBUS on > [ 4848.885324] CC1: 0 -> 0, CC2: 0 -> 3 [state TOGGLING, polarity 0, connected] > [ 4848.885372] state change TOGGLING -> SNK_ATTACH_WAIT > [ 4848.885548] pending state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED @ 200 ms > [ 4849.088240] state change SNK_ATTACH_WAIT -> SNK_DEBOUNCED [delayed 200 ms] > [ 4849.088284] state change SNK_DEBOUNCED -> SNK_ATTACHED > [ 4849.088291] polarity 1 > [ 4849.088769] Requesting mux state 1, usb-role 2, orientation 2 > [ 4849.088895] state change SNK_ATTACHED -> SNK_STARTUP > [ 4849.088907] state change SNK_STARTUP -> SNK_DISCOVERY > [ 4849.088915] Setting voltage/current limit 5000 mV 0 mA > [ 4849.088927] vbus=0 charge:=1 > [ 4849.090505] state change SNK_DISCOVERY -> SNK_WAIT_CAPABILITIES > [ 4849.090828] pending state change SNK_WAIT_CAPABILITIES -> SNK_READY @ 240 ms > [ 4849.335878] state change SNK_WAIT_CAPABILITIES -> SNK_READY [delayed 240 ms] > > You can see the next type c attach log. > It directly change state from SNK_WAIT_CAPABILITIES to SNK_READY due > to not reset hard_reset_count. > > It's easy to reproduce if you plugout USB Adapater w/i AtoC cable connected. What ever happened with this patch, is there still disagreement? thanks, greg k-h
