[v2,5/8] usb: typec: fusb302: Fix fusb302_handle_togdone_src Ra handling

[v2,5/8] usb: typec: fusb302: Fix fusb302_handle_togdone_src Ra handling

[Hans de Goede]

The FUSB302 will stop toggling with a FUSB_REG_STATUS1A_TOGSS_SRC? status,
as soon as it sees either Ra or Rd on a CC pin.

Before this commit fusb302_handle_togdone_src would assume that the toggle-
engine always stopped at the CC pin indicating the polarity, IOW it assumed
that it stopped at the pin connected to Rd. It did check the CC-status of
that pin, but it did not expect to get a CC-status of Ra and therefore
treated this as CC-open. This lead to the following 2 problems:

1) If a powered cable/adapter gets plugged in with Ra on CC1 and Rd on CC2
then 4 of 5 times when plugged in toggling will stop with a togdone_result
of FUSB_REG_STATUS1A_TOGSS_SRC1.  3/5th of the time the toggle-engine is
testing for being connected as a sink and after that it tests 1/5th of the
time for connected as a src through CC1 before finally testing the last
1/5th of the time for being a src connected through CC2.

This was a problem because we would only check the CC pin status for the
pin on which the toggling stopped which in this polarity 4 out of 5
times would be the Ra pin. The code before this commit would treat Ra as
CC-open and then restart toggling. Once toggling is restarted we are
guaranteed to end with FUSB_REG_STATUS1A_TOGSS_SRC1 as CC1 is tested first,
leading to a CC-status of Ra again and an infinite restart toggling loop.
So 4 out of 5 times when plugged in in this polarity a powered adapter
will not work.

2) Even if we happen to have the right polarity or 1/5th of the time in
the polarity with problem 1), we would report the non Rd pin as CC-open
rather then as Ra, resulting in the tcpm.c code not enabling Vconn which
is a problem for some adapters.

This commit fixes this by getting the CC-status of *both* pins and then
determining the polarity based on that, rather then on where the toggling
stopped.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/tcpm/fusb302.c | 149 ++++++++++++++++++++-----------
 1 file changed, 97 insertions(+), 52 deletions(-)

diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index f43fe34b7a73..b74c6ff67aae 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -1249,6 +1249,62 @@ static int fusb302_handle_togdone_snk(struct fusb302_chip *chip,
 	return ret;
 }
 
+/* On error returns < 0, otherwise a typec_cc_status value */
+static int fusb302_get_src_cc_status(struct fusb302_chip *chip,
+				     enum typec_cc_polarity cc_polarity,
+				     enum typec_cc_status *cc)
+{
+	u8 ra_mda = ra_mda_value[chip->src_current_status];
+	u8 rd_mda = rd_mda_value[chip->src_current_status];
+	u8 switches0_data, status0;
+	int ret;
+
+	/* Step 1: Set switches so that we measure the right CC pin */
+	switches0_data = (cc_polarity == TYPEC_POLARITY_CC1) ?
+		FUSB_REG_SWITCHES0_CC1_PU_EN | FUSB_REG_SWITCHES0_MEAS_CC1 :
+		FUSB_REG_SWITCHES0_CC2_PU_EN | FUSB_REG_SWITCHES0_MEAS_CC2;
+	ret = fusb302_i2c_write(chip, FUSB_REG_SWITCHES0, switches0_data);
+	if (ret < 0)
+		return ret;
+
+	fusb302_i2c_read(chip, FUSB_REG_SWITCHES0, &status0);
+	fusb302_log(chip, "get_src_cc_status switches: 0x%0x", status0);
+
+	/* Step 2: Set compararator volt to differentiate between Open and Rd */
+	ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, rd_mda);
+	if (ret < 0)
+		return ret;
+
+	usleep_range(5000, 10000);
+	ret = fusb302_i2c_read(chip, FUSB_REG_STATUS0, &status0);
+	if (ret < 0)
+		return ret;
+
+	fusb302_log(chip, "get_src_cc_status rd_mda status0: 0x%0x", status0);
+	if (status0 & FUSB_REG_STATUS0_COMP) {
+		*cc = TYPEC_CC_OPEN;
+		return 0;
+	}
+
+	/* Step 3: Set compararator input to differentiate between Rd and Ra. */
+	ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, ra_mda);
+	if (ret < 0)
+		return ret;
+
+	usleep_range(5000, 10000);
+	ret = fusb302_i2c_read(chip, FUSB_REG_STATUS0, &status0);
+	if (ret < 0)
+		return ret;
+
+	fusb302_log(chip, "get_src_cc_status ra_mda status0: 0x%0x", status0);
+	if (status0 & FUSB_REG_STATUS0_COMP)
+		*cc = TYPEC_CC_RD;
+	else
+		*cc = TYPEC_CC_RA;
+
+	return 0;
+}
+
 static int fusb302_handle_togdone_src(struct fusb302_chip *chip,
 				      u8 togdone_result)
 {
@@ -1259,71 +1315,62 @@ static int fusb302_handle_togdone_src(struct fusb302_chip *chip,
 	 * - set I_COMP interrupt on
 	 */
 	int ret = 0;
-	u8 status0;
-	u8 ra_mda = ra_mda_value[chip->src_current_status];
 	u8 rd_mda = rd_mda_value[chip->src_current_status];
-	bool ra_comp, rd_comp;
+	enum toggling_mode toggling_mode = chip->toggling_mode;
 	enum typec_cc_polarity cc_polarity;
-	enum typec_cc_status cc_status_active, cc1, cc2;
+	enum typec_cc_status cc1, cc2;
 
-	/* set polarity and pull_up, pull_down */
-	cc_polarity = (togdone_result == FUSB_REG_STATUS1A_TOGSS_SRC1) ?
-		      TYPEC_POLARITY_CC1 : TYPEC_POLARITY_CC2;
-	ret = fusb302_set_cc_polarity_and_pull(chip, cc_polarity, true, false);
+	/*
+	 * The toggle-engine will stop in a src state if it sees either Ra or
+	 * Rd. Determine the status for both CC pins, starting with the one
+	 * where toggling stopped, as that is where the switches point now.
+	 */
+	if (togdone_result == FUSB_REG_STATUS1A_TOGSS_SRC1)
+		ret = fusb302_get_src_cc_status(chip, TYPEC_POLARITY_CC1, &cc1);
+	else
+		ret = fusb302_get_src_cc_status(chip, TYPEC_POLARITY_CC2, &cc2);
+	if (ret < 0)
+		return ret;
+	/* we must turn off toggling before we can measure the other pin */
+	ret = fusb302_set_toggling(chip, TOGGLING_MODE_OFF);
 	if (ret < 0) {
-		fusb302_log(chip, "cannot set cc polarity %s, ret=%d",
-			    cc_polarity_name[cc_polarity], ret);
+		fusb302_log(chip, "cannot set toggling mode off, ret=%d", ret);
 		return ret;
 	}
-	/* fusb302_set_cc_polarity() has set the correct measure block */
-	ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, rd_mda);
-	if (ret < 0)
-		return ret;
-	usleep_range(50, 100);
-	ret = fusb302_i2c_read(chip, FUSB_REG_STATUS0, &status0);
+	/* get the status of the other pin */
+	if (togdone_result == FUSB_REG_STATUS1A_TOGSS_SRC1)
+		ret = fusb302_get_src_cc_status(chip, TYPEC_POLARITY_CC2, &cc2);
+	else
+		ret = fusb302_get_src_cc_status(chip, TYPEC_POLARITY_CC1, &cc1);
 	if (ret < 0)
 		return ret;
-	rd_comp = !!(status0 & FUSB_REG_STATUS0_COMP);
-	if (!rd_comp) {
-		ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, ra_mda);
-		if (ret < 0)
-			return ret;
-		usleep_range(50, 100);
-		ret = fusb302_i2c_read(chip, FUSB_REG_STATUS0, &status0);
-		if (ret < 0)
-			return ret;
-		ra_comp = !!(status0 & FUSB_REG_STATUS0_COMP);
+
+	/* determine polarity based on the status of both pins */
+	if (cc1 == TYPEC_CC_RD &&
+			(cc2 == TYPEC_CC_OPEN || cc2 == TYPEC_CC_RA)) {
+		cc_polarity = TYPEC_POLARITY_CC1;
+	} else if (cc2 == TYPEC_CC_RD &&
+		    (cc1 == TYPEC_CC_OPEN || cc1 == TYPEC_CC_RA)) {
+		cc_polarity = TYPEC_POLARITY_CC2;
+	} else {
+		fusb302_log(chip, "unexpected CC status cc1=%s, cc2=%s, restarting toggling",
+			    typec_cc_status_name[cc1],
+			    typec_cc_status_name[cc2]);
+		return fusb302_set_toggling(chip, toggling_mode);
 	}
-	if (rd_comp)
-		cc_status_active = TYPEC_CC_OPEN;
-	else if (ra_comp)
-		cc_status_active = TYPEC_CC_RD;
-	else
-		/* Ra is not supported, report as Open */
-		cc_status_active = TYPEC_CC_OPEN;
-	/* restart toggling if the cc status on the active line is OPEN */
-	if (cc_status_active == TYPEC_CC_OPEN) {
-		fusb302_log(chip, "restart toggling as CC_OPEN detected");
-		ret = fusb302_set_toggling(chip, chip->toggling_mode);
+	/* set polarity and pull_up, pull_down */
+	ret = fusb302_set_cc_polarity_and_pull(chip, cc_polarity, true, false);
+	if (ret < 0) {
+		fusb302_log(chip, "cannot set cc polarity %s, ret=%d",
+			    cc_polarity_name[cc_polarity], ret);
 		return ret;
 	}
 	/* update tcpm with the new cc value */
-	cc1 = (cc_polarity == TYPEC_POLARITY_CC1) ?
-	      cc_status_active : TYPEC_CC_OPEN;
-	cc2 = (cc_polarity == TYPEC_POLARITY_CC2) ?
-	      cc_status_active : TYPEC_CC_OPEN;
 	if ((chip->cc1 != cc1) || (chip->cc2 != cc2)) {
 		chip->cc1 = cc1;
 		chip->cc2 = cc2;
 		tcpm_cc_change(chip->tcpm_port);
 	}
-	/* turn off toggling */
-	ret = fusb302_set_toggling(chip, TOGGLING_MODE_OFF);
-	if (ret < 0) {
-		fusb302_log(chip,
-			    "cannot set toggling mode off, ret=%d", ret);
-		return ret;
-	}
 	/* set MDAC to Rd threshold, and unmask I_COMP for unplug detection */
 	ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, rd_mda);
 	if (ret < 0)
@@ -1509,10 +1556,8 @@ static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
 			    comp_result ? "true" : "false");
 		if (comp_result) {
 			/* cc level > Rd_threashold, detach */
-			if (chip->cc_polarity == TYPEC_POLARITY_CC1)
-				chip->cc1 = TYPEC_CC_OPEN;
-			else
-				chip->cc2 = TYPEC_CC_OPEN;
+			chip->cc1 = TYPEC_CC_OPEN;
+			chip->cc2 = TYPEC_CC_OPEN;
 			tcpm_cc_change(chip->tcpm_port);
 		}
 	}

[v2,5/8] usb: typec: fusb302: Fix fusb302_handle_togdone_src Ra handling

[Hans de Goede]

Hi,

On 07-03-19 19:19, Guenter Roeck wrote:
> On Thu, Mar 07, 2019 at 05:36:04PM +0100, Hans de Goede wrote:
>> The FUSB302 will stop toggling with a FUSB_REG_STATUS1A_TOGSS_SRC? status,
>> as soon as it sees either Ra or Rd on a CC pin.
>>
>> Before this commit fusb302_handle_togdone_src would assume that the toggle-
>> engine always stopped at the CC pin indicating the polarity, IOW it assumed
>> that it stopped at the pin connected to Rd. It did check the CC-status of
>> that pin, but it did not expect to get a CC-status of Ra and therefore
>> treated this as CC-open. This lead to the following 2 problems:
>>
>> 1) If a powered cable/adapter gets plugged in with Ra on CC1 and Rd on CC2
>> then 4 of 5 times when plugged in toggling will stop with a togdone_result
>> of FUSB_REG_STATUS1A_TOGSS_SRC1.  3/5th of the time the toggle-engine is
>> testing for being connected as a sink and after that it tests 1/5th of the
>> time for connected as a src through CC1 before finally testing the last
>> 1/5th of the time for being a src connected through CC2.
>>
>> This was a problem because we would only check the CC pin status for the
>> pin on which the toggling stopped which in this polarity 4 out of 5
>> times would be the Ra pin. The code before this commit would treat Ra as
>> CC-open and then restart toggling. Once toggling is restarted we are
>> guaranteed to end with FUSB_REG_STATUS1A_TOGSS_SRC1 as CC1 is tested first,
>> leading to a CC-status of Ra again and an infinite restart toggling loop.
>> So 4 out of 5 times when plugged in in this polarity a powered adapter
>> will not work.
>>
>> 2) Even if we happen to have the right polarity or 1/5th of the time in
>> the polarity with problem 1), we would report the non Rd pin as CC-open
>> rather then as Ra, resulting in the tcpm.c code not enabling Vconn which
>> is a problem for some adapters.
>>
>> This commit fixes this by getting the CC-status of *both* pins and then
>> determining the polarity based on that, rather then on where the toggling
>> stopped.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/usb/typec/tcpm/fusb302.c | 149 ++++++++++++++++++++-----------
>>   1 file changed, 97 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
>> index f43fe34b7a73..b74c6ff67aae 100644
>> --- a/drivers/usb/typec/tcpm/fusb302.c
>> +++ b/drivers/usb/typec/tcpm/fusb302.c
>> @@ -1249,6 +1249,62 @@ static int fusb302_handle_togdone_snk(struct fusb302_chip *chip,
>>   	return ret;
>>   }
>>   
>> +/* On error returns < 0, otherwise a typec_cc_status value */
>> +static int fusb302_get_src_cc_status(struct fusb302_chip *chip,
>> +				     enum typec_cc_polarity cc_polarity,
>> +				     enum typec_cc_status *cc)
>> +{
>> +	u8 ra_mda = ra_mda_value[chip->src_current_status];
>> +	u8 rd_mda = rd_mda_value[chip->src_current_status];
>> +	u8 switches0_data, status0;
>> +	int ret;
>> +
>> +	/* Step 1: Set switches so that we measure the right CC pin */
>> +	switches0_data = (cc_polarity == TYPEC_POLARITY_CC1) ?
>> +		FUSB_REG_SWITCHES0_CC1_PU_EN | FUSB_REG_SWITCHES0_MEAS_CC1 :
>> +		FUSB_REG_SWITCHES0_CC2_PU_EN | FUSB_REG_SWITCHES0_MEAS_CC2;
>> +	ret = fusb302_i2c_write(chip, FUSB_REG_SWITCHES0, switches0_data);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	fusb302_i2c_read(chip, FUSB_REG_SWITCHES0, &status0);
>> +	fusb302_log(chip, "get_src_cc_status switches: 0x%0x", status0);
>> +
>> +	/* Step 2: Set compararator volt to differentiate between Open and Rd */
>> +	ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, rd_mda);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	usleep_range(5000, 10000);
> 
> This and the second sleep below results in significant delays of up to
> 20 ms. This is significantly more than before. Are you sure this doesn't
> result in some timing violations ?

Ugh, that line should be:

	usleep_range(50, 100);

(and again below).

I added the 2 extra zeroes while debugging this code, but the actual bug
I had then in the code was different. I actually remember removing the zeroes
again, but clearly they somehow got back; or I remember wrongly.

Anyways I will do a v3 with this fixed, hopefully later today, but I
need to re-run all my tests, so this may take a while.

 > And is that really the only possible
 > solution to address the problem you observed ?

I have been unable to come up with another way to deal with the toggle
engine stopping on the first Cc pin with a pull-down attached, be it the
Rd or Ra one.

But I should be able to get the delay down to twice the old delay,
instead of the up to 20 ms.

Regards,

Hans

p.s.

Thank you for the review!

>> +	ret = fusb302_i2c_read(chip, FUSB_REG_STATUS0, &status0);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	fusb302_log(chip, "get_src_cc_status rd_mda status0: 0x%0x", status0);
>> +	if (status0 & FUSB_REG_STATUS0_COMP) {
>> +		*cc = TYPEC_CC_OPEN;
>> +		return 0;
>> +	}
>> +
>> +	/* Step 3: Set compararator input to differentiate between Rd and Ra. */
>> +	ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, ra_mda);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	usleep_range(5000, 10000);
>> +	ret = fusb302_i2c_read(chip, FUSB_REG_STATUS0, &status0);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	fusb302_log(chip, "get_src_cc_status ra_mda status0: 0x%0x", status0);
>> +	if (status0 & FUSB_REG_STATUS0_COMP)
>> +		*cc = TYPEC_CC_RD;
>> +	else
>> +		*cc = TYPEC_CC_RA;
>> +
>> +	return 0;
>> +}
>> +
>>   static int fusb302_handle_togdone_src(struct fusb302_chip *chip,
>>   				      u8 togdone_result)
>>   {
>> @@ -1259,71 +1315,62 @@ static int fusb302_handle_togdone_src(struct fusb302_chip *chip,
>>   	 * - set I_COMP interrupt on
>>   	 */
>>   	int ret = 0;
>> -	u8 status0;
>> -	u8 ra_mda = ra_mda_value[chip->src_current_status];
>>   	u8 rd_mda = rd_mda_value[chip->src_current_status];
>> -	bool ra_comp, rd_comp;
>> +	enum toggling_mode toggling_mode = chip->toggling_mode;
>>   	enum typec_cc_polarity cc_polarity;
>> -	enum typec_cc_status cc_status_active, cc1, cc2;
>> +	enum typec_cc_status cc1, cc2;
>>   
>> -	/* set polarity and pull_up, pull_down */
>> -	cc_polarity = (togdone_result == FUSB_REG_STATUS1A_TOGSS_SRC1) ?
>> -		      TYPEC_POLARITY_CC1 : TYPEC_POLARITY_CC2;
>> -	ret = fusb302_set_cc_polarity_and_pull(chip, cc_polarity, true, false);
>> +	/*
>> +	 * The toggle-engine will stop in a src state if it sees either Ra or
>> +	 * Rd. Determine the status for both CC pins, starting with the one
>> +	 * where toggling stopped, as that is where the switches point now.
>> +	 */
>> +	if (togdone_result == FUSB_REG_STATUS1A_TOGSS_SRC1)
>> +		ret = fusb302_get_src_cc_status(chip, TYPEC_POLARITY_CC1, &cc1);
>> +	else
>> +		ret = fusb302_get_src_cc_status(chip, TYPEC_POLARITY_CC2, &cc2);
>> +	if (ret < 0)
>> +		return ret;
>> +	/* we must turn off toggling before we can measure the other pin */
>> +	ret = fusb302_set_toggling(chip, TOGGLING_MODE_OFF);
>>   	if (ret < 0) {
>> -		fusb302_log(chip, "cannot set cc polarity %s, ret=%d",
>> -			    cc_polarity_name[cc_polarity], ret);
>> +		fusb302_log(chip, "cannot set toggling mode off, ret=%d", ret);
>>   		return ret;
>>   	}
>> -	/* fusb302_set_cc_polarity() has set the correct measure block */
>> -	ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, rd_mda);
>> -	if (ret < 0)
>> -		return ret;
>> -	usleep_range(50, 100);
>> -	ret = fusb302_i2c_read(chip, FUSB_REG_STATUS0, &status0);
>> +	/* get the status of the other pin */
>> +	if (togdone_result == FUSB_REG_STATUS1A_TOGSS_SRC1)
>> +		ret = fusb302_get_src_cc_status(chip, TYPEC_POLARITY_CC2, &cc2);
>> +	else
>> +		ret = fusb302_get_src_cc_status(chip, TYPEC_POLARITY_CC1, &cc1);
>>   	if (ret < 0)
>>   		return ret;
>> -	rd_comp = !!(status0 & FUSB_REG_STATUS0_COMP);
>> -	if (!rd_comp) {
>> -		ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, ra_mda);
>> -		if (ret < 0)
>> -			return ret;
>> -		usleep_range(50, 100);
>> -		ret = fusb302_i2c_read(chip, FUSB_REG_STATUS0, &status0);
>> -		if (ret < 0)
>> -			return ret;
>> -		ra_comp = !!(status0 & FUSB_REG_STATUS0_COMP);
>> +
>> +	/* determine polarity based on the status of both pins */
>> +	if (cc1 == TYPEC_CC_RD &&
>> +			(cc2 == TYPEC_CC_OPEN || cc2 == TYPEC_CC_RA)) {
>> +		cc_polarity = TYPEC_POLARITY_CC1;
>> +	} else if (cc2 == TYPEC_CC_RD &&
>> +		    (cc1 == TYPEC_CC_OPEN || cc1 == TYPEC_CC_RA)) {
>> +		cc_polarity = TYPEC_POLARITY_CC2;
>> +	} else {
>> +		fusb302_log(chip, "unexpected CC status cc1=%s, cc2=%s, restarting toggling",
>> +			    typec_cc_status_name[cc1],
>> +			    typec_cc_status_name[cc2]);
>> +		return fusb302_set_toggling(chip, toggling_mode);
>>   	}
>> -	if (rd_comp)
>> -		cc_status_active = TYPEC_CC_OPEN;
>> -	else if (ra_comp)
>> -		cc_status_active = TYPEC_CC_RD;
>> -	else
>> -		/* Ra is not supported, report as Open */
>> -		cc_status_active = TYPEC_CC_OPEN;
>> -	/* restart toggling if the cc status on the active line is OPEN */
>> -	if (cc_status_active == TYPEC_CC_OPEN) {
>> -		fusb302_log(chip, "restart toggling as CC_OPEN detected");
>> -		ret = fusb302_set_toggling(chip, chip->toggling_mode);
>> +	/* set polarity and pull_up, pull_down */
>> +	ret = fusb302_set_cc_polarity_and_pull(chip, cc_polarity, true, false);
>> +	if (ret < 0) {
>> +		fusb302_log(chip, "cannot set cc polarity %s, ret=%d",
>> +			    cc_polarity_name[cc_polarity], ret);
>>   		return ret;
>>   	}
>>   	/* update tcpm with the new cc value */
>> -	cc1 = (cc_polarity == TYPEC_POLARITY_CC1) ?
>> -	      cc_status_active : TYPEC_CC_OPEN;
>> -	cc2 = (cc_polarity == TYPEC_POLARITY_CC2) ?
>> -	      cc_status_active : TYPEC_CC_OPEN;
>>   	if ((chip->cc1 != cc1) || (chip->cc2 != cc2)) {
>>   		chip->cc1 = cc1;
>>   		chip->cc2 = cc2;
>>   		tcpm_cc_change(chip->tcpm_port);
>>   	}
>> -	/* turn off toggling */
>> -	ret = fusb302_set_toggling(chip, TOGGLING_MODE_OFF);
>> -	if (ret < 0) {
>> -		fusb302_log(chip,
>> -			    "cannot set toggling mode off, ret=%d", ret);
>> -		return ret;
>> -	}
>>   	/* set MDAC to Rd threshold, and unmask I_COMP for unplug detection */
>>   	ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, rd_mda);
>>   	if (ret < 0)
>> @@ -1509,10 +1556,8 @@ static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
>>   			    comp_result ? "true" : "false");
>>   		if (comp_result) {
>>   			/* cc level > Rd_threashold, detach */
>> -			if (chip->cc_polarity == TYPEC_POLARITY_CC1)
>> -				chip->cc1 = TYPEC_CC_OPEN;
>> -			else
>> -				chip->cc2 = TYPEC_CC_OPEN;
>> +			chip->cc1 = TYPEC_CC_OPEN;
>> +			chip->cc2 = TYPEC_CC_OPEN;
>>   			tcpm_cc_change(chip->tcpm_port);
>>   		}
>>   	}
>> -- 
>> 2.20.1
>>

[v2,5/8] usb: typec: fusb302: Fix fusb302_handle_togdone_src Ra handling

[Guenter Roeck]

On Thu, Mar 07, 2019 at 05:36:04PM +0100, Hans de Goede wrote:
> The FUSB302 will stop toggling with a FUSB_REG_STATUS1A_TOGSS_SRC? status,
> as soon as it sees either Ra or Rd on a CC pin.
> 
> Before this commit fusb302_handle_togdone_src would assume that the toggle-
> engine always stopped at the CC pin indicating the polarity, IOW it assumed
> that it stopped at the pin connected to Rd. It did check the CC-status of
> that pin, but it did not expect to get a CC-status of Ra and therefore
> treated this as CC-open. This lead to the following 2 problems:
> 
> 1) If a powered cable/adapter gets plugged in with Ra on CC1 and Rd on CC2
> then 4 of 5 times when plugged in toggling will stop with a togdone_result
> of FUSB_REG_STATUS1A_TOGSS_SRC1.  3/5th of the time the toggle-engine is
> testing for being connected as a sink and after that it tests 1/5th of the
> time for connected as a src through CC1 before finally testing the last
> 1/5th of the time for being a src connected through CC2.
> 
> This was a problem because we would only check the CC pin status for the
> pin on which the toggling stopped which in this polarity 4 out of 5
> times would be the Ra pin. The code before this commit would treat Ra as
> CC-open and then restart toggling. Once toggling is restarted we are
> guaranteed to end with FUSB_REG_STATUS1A_TOGSS_SRC1 as CC1 is tested first,
> leading to a CC-status of Ra again and an infinite restart toggling loop.
> So 4 out of 5 times when plugged in in this polarity a powered adapter
> will not work.
> 
> 2) Even if we happen to have the right polarity or 1/5th of the time in
> the polarity with problem 1), we would report the non Rd pin as CC-open
> rather then as Ra, resulting in the tcpm.c code not enabling Vconn which
> is a problem for some adapters.
> 
> This commit fixes this by getting the CC-status of *both* pins and then
> determining the polarity based on that, rather then on where the toggling
> stopped.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/typec/tcpm/fusb302.c | 149 ++++++++++++++++++++-----------
>  1 file changed, 97 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index f43fe34b7a73..b74c6ff67aae 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -1249,6 +1249,62 @@ static int fusb302_handle_togdone_snk(struct fusb302_chip *chip,
>  	return ret;
>  }
>  
> +/* On error returns < 0, otherwise a typec_cc_status value */
> +static int fusb302_get_src_cc_status(struct fusb302_chip *chip,
> +				     enum typec_cc_polarity cc_polarity,
> +				     enum typec_cc_status *cc)
> +{
> +	u8 ra_mda = ra_mda_value[chip->src_current_status];
> +	u8 rd_mda = rd_mda_value[chip->src_current_status];
> +	u8 switches0_data, status0;
> +	int ret;
> +
> +	/* Step 1: Set switches so that we measure the right CC pin */
> +	switches0_data = (cc_polarity == TYPEC_POLARITY_CC1) ?
> +		FUSB_REG_SWITCHES0_CC1_PU_EN | FUSB_REG_SWITCHES0_MEAS_CC1 :
> +		FUSB_REG_SWITCHES0_CC2_PU_EN | FUSB_REG_SWITCHES0_MEAS_CC2;
> +	ret = fusb302_i2c_write(chip, FUSB_REG_SWITCHES0, switches0_data);
> +	if (ret < 0)
> +		return ret;
> +
> +	fusb302_i2c_read(chip, FUSB_REG_SWITCHES0, &status0);
> +	fusb302_log(chip, "get_src_cc_status switches: 0x%0x", status0);
> +
> +	/* Step 2: Set compararator volt to differentiate between Open and Rd */
> +	ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, rd_mda);
> +	if (ret < 0)
> +		return ret;
> +
> +	usleep_range(5000, 10000);

This and the second sleep below results in significant delays of up to
20 ms. This is significantly more than before. Are you sure this doesn't
result in some timing violations ? And is that really the only possible
solution to address the problem you observed ?

Thanks,
Guenter

> +	ret = fusb302_i2c_read(chip, FUSB_REG_STATUS0, &status0);
> +	if (ret < 0)
> +		return ret;
> +
> +	fusb302_log(chip, "get_src_cc_status rd_mda status0: 0x%0x", status0);
> +	if (status0 & FUSB_REG_STATUS0_COMP) {
> +		*cc = TYPEC_CC_OPEN;
> +		return 0;
> +	}
> +
> +	/* Step 3: Set compararator input to differentiate between Rd and Ra. */
> +	ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, ra_mda);
> +	if (ret < 0)
> +		return ret;
> +
> +	usleep_range(5000, 10000);
> +	ret = fusb302_i2c_read(chip, FUSB_REG_STATUS0, &status0);
> +	if (ret < 0)
> +		return ret;
> +
> +	fusb302_log(chip, "get_src_cc_status ra_mda status0: 0x%0x", status0);
> +	if (status0 & FUSB_REG_STATUS0_COMP)
> +		*cc = TYPEC_CC_RD;
> +	else
> +		*cc = TYPEC_CC_RA;
> +
> +	return 0;
> +}
> +
>  static int fusb302_handle_togdone_src(struct fusb302_chip *chip,
>  				      u8 togdone_result)
>  {
> @@ -1259,71 +1315,62 @@ static int fusb302_handle_togdone_src(struct fusb302_chip *chip,
>  	 * - set I_COMP interrupt on
>  	 */
>  	int ret = 0;
> -	u8 status0;
> -	u8 ra_mda = ra_mda_value[chip->src_current_status];
>  	u8 rd_mda = rd_mda_value[chip->src_current_status];
> -	bool ra_comp, rd_comp;
> +	enum toggling_mode toggling_mode = chip->toggling_mode;
>  	enum typec_cc_polarity cc_polarity;
> -	enum typec_cc_status cc_status_active, cc1, cc2;
> +	enum typec_cc_status cc1, cc2;
>  
> -	/* set polarity and pull_up, pull_down */
> -	cc_polarity = (togdone_result == FUSB_REG_STATUS1A_TOGSS_SRC1) ?
> -		      TYPEC_POLARITY_CC1 : TYPEC_POLARITY_CC2;
> -	ret = fusb302_set_cc_polarity_and_pull(chip, cc_polarity, true, false);
> +	/*
> +	 * The toggle-engine will stop in a src state if it sees either Ra or
> +	 * Rd. Determine the status for both CC pins, starting with the one
> +	 * where toggling stopped, as that is where the switches point now.
> +	 */
> +	if (togdone_result == FUSB_REG_STATUS1A_TOGSS_SRC1)
> +		ret = fusb302_get_src_cc_status(chip, TYPEC_POLARITY_CC1, &cc1);
> +	else
> +		ret = fusb302_get_src_cc_status(chip, TYPEC_POLARITY_CC2, &cc2);
> +	if (ret < 0)
> +		return ret;
> +	/* we must turn off toggling before we can measure the other pin */
> +	ret = fusb302_set_toggling(chip, TOGGLING_MODE_OFF);
>  	if (ret < 0) {
> -		fusb302_log(chip, "cannot set cc polarity %s, ret=%d",
> -			    cc_polarity_name[cc_polarity], ret);
> +		fusb302_log(chip, "cannot set toggling mode off, ret=%d", ret);
>  		return ret;
>  	}
> -	/* fusb302_set_cc_polarity() has set the correct measure block */
> -	ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, rd_mda);
> -	if (ret < 0)
> -		return ret;
> -	usleep_range(50, 100);
> -	ret = fusb302_i2c_read(chip, FUSB_REG_STATUS0, &status0);
> +	/* get the status of the other pin */
> +	if (togdone_result == FUSB_REG_STATUS1A_TOGSS_SRC1)
> +		ret = fusb302_get_src_cc_status(chip, TYPEC_POLARITY_CC2, &cc2);
> +	else
> +		ret = fusb302_get_src_cc_status(chip, TYPEC_POLARITY_CC1, &cc1);
>  	if (ret < 0)
>  		return ret;
> -	rd_comp = !!(status0 & FUSB_REG_STATUS0_COMP);
> -	if (!rd_comp) {
> -		ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, ra_mda);
> -		if (ret < 0)
> -			return ret;
> -		usleep_range(50, 100);
> -		ret = fusb302_i2c_read(chip, FUSB_REG_STATUS0, &status0);
> -		if (ret < 0)
> -			return ret;
> -		ra_comp = !!(status0 & FUSB_REG_STATUS0_COMP);
> +
> +	/* determine polarity based on the status of both pins */
> +	if (cc1 == TYPEC_CC_RD &&
> +			(cc2 == TYPEC_CC_OPEN || cc2 == TYPEC_CC_RA)) {
> +		cc_polarity = TYPEC_POLARITY_CC1;
> +	} else if (cc2 == TYPEC_CC_RD &&
> +		    (cc1 == TYPEC_CC_OPEN || cc1 == TYPEC_CC_RA)) {
> +		cc_polarity = TYPEC_POLARITY_CC2;
> +	} else {
> +		fusb302_log(chip, "unexpected CC status cc1=%s, cc2=%s, restarting toggling",
> +			    typec_cc_status_name[cc1],
> +			    typec_cc_status_name[cc2]);
> +		return fusb302_set_toggling(chip, toggling_mode);
>  	}
> -	if (rd_comp)
> -		cc_status_active = TYPEC_CC_OPEN;
> -	else if (ra_comp)
> -		cc_status_active = TYPEC_CC_RD;
> -	else
> -		/* Ra is not supported, report as Open */
> -		cc_status_active = TYPEC_CC_OPEN;
> -	/* restart toggling if the cc status on the active line is OPEN */
> -	if (cc_status_active == TYPEC_CC_OPEN) {
> -		fusb302_log(chip, "restart toggling as CC_OPEN detected");
> -		ret = fusb302_set_toggling(chip, chip->toggling_mode);
> +	/* set polarity and pull_up, pull_down */
> +	ret = fusb302_set_cc_polarity_and_pull(chip, cc_polarity, true, false);
> +	if (ret < 0) {
> +		fusb302_log(chip, "cannot set cc polarity %s, ret=%d",
> +			    cc_polarity_name[cc_polarity], ret);
>  		return ret;
>  	}
>  	/* update tcpm with the new cc value */
> -	cc1 = (cc_polarity == TYPEC_POLARITY_CC1) ?
> -	      cc_status_active : TYPEC_CC_OPEN;
> -	cc2 = (cc_polarity == TYPEC_POLARITY_CC2) ?
> -	      cc_status_active : TYPEC_CC_OPEN;
>  	if ((chip->cc1 != cc1) || (chip->cc2 != cc2)) {
>  		chip->cc1 = cc1;
>  		chip->cc2 = cc2;
>  		tcpm_cc_change(chip->tcpm_port);
>  	}
> -	/* turn off toggling */
> -	ret = fusb302_set_toggling(chip, TOGGLING_MODE_OFF);
> -	if (ret < 0) {
> -		fusb302_log(chip,
> -			    "cannot set toggling mode off, ret=%d", ret);
> -		return ret;
> -	}
>  	/* set MDAC to Rd threshold, and unmask I_COMP for unplug detection */
>  	ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, rd_mda);
>  	if (ret < 0)
> @@ -1509,10 +1556,8 @@ static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
>  			    comp_result ? "true" : "false");
>  		if (comp_result) {
>  			/* cc level > Rd_threashold, detach */
> -			if (chip->cc_polarity == TYPEC_POLARITY_CC1)
> -				chip->cc1 = TYPEC_CC_OPEN;
> -			else
> -				chip->cc2 = TYPEC_CC_OPEN;
> +			chip->cc1 = TYPEC_CC_OPEN;
> +			chip->cc2 = TYPEC_CC_OPEN;
>  			tcpm_cc_change(chip->tcpm_port);
>  		}
>  	}
> -- 
> 2.20.1
>