[PATCH 0/3] usb: typec: tcpm: Add some FAULT_STATUS processing

[PATCH 0/3] usb: typec: tcpm: Add some FAULT_STATUS processing

[Angus Ainslie (Purism)]

A high number of interrupts were being generated by the tcpci driver.
These patches put debugging info into the tcpm log and clear the
interrupt condition.

Angus Ainslie (Purism) (3):
  usb: typec: tcpm: Export the logging function so it can be used by
    tcpci
  usb: typec: tcpm: Add functions to read the VBUS voltage
  usb: typec: tcpm: Clear the fault status register

 drivers/usb/typec/tcpm/tcpci.c | 56 ++++++++++++++++++++++++++++++++++
 drivers/usb/typec/tcpm/tcpm.c  |  3 +-
 include/linux/usb/tcpm.h       |  2 ++
 3 files changed, 60 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH 1/3] usb: typec: tcpm: Export the logging function so it can be used by tcpci

[Angus Ainslie (Purism)]

Give the rest of the tcpm stack access to the logging function.

Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
---
 drivers/usb/typec/tcpm/tcpm.c | 3 ++-
 include/linux/usb/tcpm.h      | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index fba32d84e578..7c35cc1accae 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -465,7 +465,7 @@ static void _tcpm_log(struct tcpm_port *port, const char *fmt, va_list args)
 }
 
 __printf(2, 3)
-static void tcpm_log(struct tcpm_port *port, const char *fmt, ...)
+void tcpm_log(struct tcpm_port *port, const char *fmt, ...)
 {
 	va_list args;
 
@@ -479,6 +479,7 @@ static void tcpm_log(struct tcpm_port *port, const char *fmt, ...)
 	_tcpm_log(port, fmt, args);
 	va_end(args);
 }
+EXPORT_SYMBOL_GPL(tcpm_log);
 
 __printf(2, 3)
 static void tcpm_log_force(struct tcpm_port *port, const char *fmt, ...)
diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
index 36a15dcadc53..70bfffdf5760 100644
--- a/include/linux/usb/tcpm.h
+++ b/include/linux/usb/tcpm.h
@@ -169,4 +169,6 @@ void tcpm_pd_transmit_complete(struct tcpm_port *port,
 void tcpm_pd_hard_reset(struct tcpm_port *port);
 void tcpm_tcpc_reset(struct tcpm_port *port);
 
+void tcpm_log(struct tcpm_port *port, const char *fmt, ...);
+
 #endif /* __LINUX_USB_TCPM_H */
-- 
2.17.1

Re: [PATCH 1/3] usb: typec: tcpm: Export the logging function so it can be used by tcpci

[Guenter Roeck]

On Mon, May 06, 2019 at 08:08:28AM -0600, Angus Ainslie (Purism) wrote:
> Give the rest of the tcpm stack access to the logging function.
> 

Those functions are not intended to be exported. I would be open to extracting
the loging code into its own file and make it generic/usable for all tcpm
drivers, but the tcpm log itself should be limited to tcpm, and remain so.

Guenter

> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 3 ++-
>  include/linux/usb/tcpm.h      | 2 ++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index fba32d84e578..7c35cc1accae 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -465,7 +465,7 @@ static void _tcpm_log(struct tcpm_port *port, const char *fmt, va_list args)
>  }
>  
>  __printf(2, 3)
> -static void tcpm_log(struct tcpm_port *port, const char *fmt, ...)
> +void tcpm_log(struct tcpm_port *port, const char *fmt, ...)
>  {
>  	va_list args;
>  
> @@ -479,6 +479,7 @@ static void tcpm_log(struct tcpm_port *port, const char *fmt, ...)
>  	_tcpm_log(port, fmt, args);
>  	va_end(args);
>  }
> +EXPORT_SYMBOL_GPL(tcpm_log);
>  
>  __printf(2, 3)
>  static void tcpm_log_force(struct tcpm_port *port, const char *fmt, ...)
> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> index 36a15dcadc53..70bfffdf5760 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -169,4 +169,6 @@ void tcpm_pd_transmit_complete(struct tcpm_port *port,
>  void tcpm_pd_hard_reset(struct tcpm_port *port);
>  void tcpm_tcpc_reset(struct tcpm_port *port);
>  
> +void tcpm_log(struct tcpm_port *port, const char *fmt, ...);
> +
>  #endif /* __LINUX_USB_TCPM_H */
> -- 
> 2.17.1
> 

[PATCH 2/3] usb: typec: tcpm: Add functions to read the VBUS voltage

[Angus Ainslie (Purism)]

Put some diagnostics in the tcpm log when there's an over
or under voltage situation.

Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
---
 drivers/usb/typec/tcpm/tcpci.c | 44 ++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index c1f7073a56de..c6e0e48b9a2a 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -261,6 +261,39 @@ static int tcpci_set_pd_rx(struct tcpc_dev *tcpc, bool enable)
 	return 0;
 }
 
+static int tcpci_get_vbus_voltage(struct tcpc_dev *tcpc)
+{
+	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
+	u16 vbus_reg;
+	unsigned int vbus_voltage;
+	int ret, scale;
+
+	ret = tcpci_read16(tcpci, TCPC_VBUS_VOLTAGE, &vbus_reg);
+	if (ret < 0)
+		return ret;
+
+	vbus_voltage = vbus_reg & 0x3f;
+	switch ((ret >> 10) & 3) {
+	case 0:
+		scale = 1;
+		break;
+	case 1:
+		scale = 2;
+		break;
+	case 2:
+		scale = 4;
+		break;
+	case 3:
+		tcpm_log(tcpci->port, "invalid VBUS scale");
+		return -1;
+	}
+
+	if (scale != 1)
+		vbus_voltage *= scale;
+
+	return vbus_voltage;
+}
+
 static int tcpci_get_vbus(struct tcpc_dev *tcpc)
 {
 	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
@@ -463,6 +496,17 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
 	else if (status & TCPC_ALERT_TX_FAILED)
 		tcpm_pd_transmit_complete(tcpci->port, TCPC_TX_FAILED);
 
+	if (status & (TCPC_ALERT_V_ALARM_LO | TCPC_ALERT_V_ALARM_HI)) {
+		int ret;
+
+		ret = tcpci_get_vbus_voltage(&tcpci->tcpc);
+
+		if (IS_ERR(ret))
+			tcpm_log(tcpci->port, "Can't read VBUS voltage");
+		else
+			tcpm_log(tcpci->port, "Invalid VBUS voltage %d", ret);
+	}
+
 	return IRQ_HANDLED;
 }
 EXPORT_SYMBOL_GPL(tcpci_irq);
-- 
2.17.1

Re: [PATCH 2/3] usb: typec: tcpm: Add functions to read the VBUS voltage

[Guenter Roeck]

On Mon, May 06, 2019 at 08:08:29AM -0600, Angus Ainslie (Purism) wrote:
> Put some diagnostics in the tcpm log when there's an over
> or under voltage situation.
> 
> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>

Subject is missing 'tcpci'.

> ---
>  drivers/usb/typec/tcpm/tcpci.c | 44 ++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index c1f7073a56de..c6e0e48b9a2a 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -261,6 +261,39 @@ static int tcpci_set_pd_rx(struct tcpc_dev *tcpc, bool enable)
>  	return 0;
>  }
>  
> +static int tcpci_get_vbus_voltage(struct tcpc_dev *tcpc)
> +{
> +	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> +	u16 vbus_reg;
> +	unsigned int vbus_voltage;
> +	int ret, scale;
> +
> +	ret = tcpci_read16(tcpci, TCPC_VBUS_VOLTAGE, &vbus_reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	vbus_voltage = vbus_reg & 0x3f;
> +	switch ((ret >> 10) & 3) {

Did you test this code ?

> +	case 0:
> +		scale = 1;
> +		break;
> +	case 1:
> +		scale = 2;
> +		break;
> +	case 2:
> +		scale = 4;
> +		break;
> +	case 3:
> +		tcpm_log(tcpci->port, "invalid VBUS scale");
> +		return -1;

Any special reason for not using standard error codes ?
The code above does, meaning this is a hardcodesd -EPERM, which doesn't
really make any sense.

> +	}
> +
> +	if (scale != 1)
> +		vbus_voltage *= scale;

I don't immediately see why this is better than, say,

	scale = (vbus_reg >> 10) & 3;
	if (scale == 3)
		return -Esomething;	// -EPROTO, maybe
	return vbus_voltage << scale;

> +
> +	return vbus_voltage;
> +}
> +
>  static int tcpci_get_vbus(struct tcpc_dev *tcpc)
>  {
>  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> @@ -463,6 +496,17 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  	else if (status & TCPC_ALERT_TX_FAILED)
>  		tcpm_pd_transmit_complete(tcpci->port, TCPC_TX_FAILED);
>  
> +	if (status & (TCPC_ALERT_V_ALARM_LO | TCPC_ALERT_V_ALARM_HI)) {
> +		int ret;
> +
> +		ret = tcpci_get_vbus_voltage(&tcpci->tcpc);
> +
Unnecessary empty line.

> +		if (IS_ERR(ret))
> +			tcpm_log(tcpci->port, "Can't read VBUS voltage");

VBUS_VOLTAGE is an optional register. This is not an error. Besides, the
message doesn't match the event and is useless.

> +		else
> +			tcpm_log(tcpci->port, "Invalid VBUS voltage %d", ret);

Displaying a raw number without context is not very useful.
'ret' is the voltage in multiples of 25mV. Besides, the error is that a low
or high voltage was detected. That doesn't mean the voltage is still invalid.
The error message should reflect that situation. Something like

		"VBUS {low, high} detected, VBUS=x.yy V"

would be much more useful (with VBUS=x.yy being optional).

Also, please no tcpm log. The tcpci driver needs to implement
its own logging if that is desired.

> +	}
> +
>  	return IRQ_HANDLED;
>  }
>  EXPORT_SYMBOL_GPL(tcpci_irq);
> -- 
> 2.17.1
> 

Re: [PATCH 2/3] usb: typec: tcpm: Add functions to read the VBUS voltage

[Angus Ainslie]

On 2019-05-06 10:20, Guenter Roeck wrote:
> On Mon, May 06, 2019 at 08:08:29AM -0600, Angus Ainslie (Purism) wrote:
>> Put some diagnostics in the tcpm log when there's an over
>> or under voltage situation.
>> 
>> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> 
> Subject is missing 'tcpci'.
> 
>> ---
>>  drivers/usb/typec/tcpm/tcpci.c | 44 
>> ++++++++++++++++++++++++++++++++++
>>  1 file changed, 44 insertions(+)
>> 
>> diff --git a/drivers/usb/typec/tcpm/tcpci.c 
>> b/drivers/usb/typec/tcpm/tcpci.c
>> index c1f7073a56de..c6e0e48b9a2a 100644
>> --- a/drivers/usb/typec/tcpm/tcpci.c
>> +++ b/drivers/usb/typec/tcpm/tcpci.c
>> @@ -261,6 +261,39 @@ static int tcpci_set_pd_rx(struct tcpc_dev *tcpc, 
>> bool enable)
>>  	return 0;
>>  }
>> 
>> +static int tcpci_get_vbus_voltage(struct tcpc_dev *tcpc)
>> +{
>> +	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>> +	u16 vbus_reg;
>> +	unsigned int vbus_voltage;
>> +	int ret, scale;
>> +
>> +	ret = tcpci_read16(tcpci, TCPC_VBUS_VOLTAGE, &vbus_reg);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	vbus_voltage = vbus_reg & 0x3f;
>> +	switch ((ret >> 10) & 3) {
> 
> Did you test this code ?
> 

It turned out this wasn't how the device was failing so the code path 
never got executed. I'll figure out how to get it to run before v2.

>> +	case 0:
>> +		scale = 1;
>> +		break;
>> +	case 1:
>> +		scale = 2;
>> +		break;
>> +	case 2:
>> +		scale = 4;
>> +		break;
>> +	case 3:
>> +		tcpm_log(tcpci->port, "invalid VBUS scale");
>> +		return -1;
> 
> Any special reason for not using standard error codes ?
> The code above does, meaning this is a hardcodesd -EPERM, which doesn't
> really make any sense.
> 

Ok I'll find a better return value.

>> +	}
>> +
>> +	if (scale != 1)
>> +		vbus_voltage *= scale;
> 
> I don't immediately see why this is better than, say,
> 
> 	scale = (vbus_reg >> 10) & 3;
> 	if (scale == 3)
> 		return -Esomething;	// -EPROTO, maybe
> 	return vbus_voltage << scale;
> 

That looks more concise than what I can up with.

>> +
>> +	return vbus_voltage;
>> +}
>> +
>>  static int tcpci_get_vbus(struct tcpc_dev *tcpc)
>>  {
>>  	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>> @@ -463,6 +496,17 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>>  	else if (status & TCPC_ALERT_TX_FAILED)
>>  		tcpm_pd_transmit_complete(tcpci->port, TCPC_TX_FAILED);
>> 
>> +	if (status & (TCPC_ALERT_V_ALARM_LO | TCPC_ALERT_V_ALARM_HI)) {
>> +		int ret;
>> +
>> +		ret = tcpci_get_vbus_voltage(&tcpci->tcpc);
>> +
> Unnecessary empty line.
> 
>> +		if (IS_ERR(ret))
>> +			tcpm_log(tcpci->port, "Can't read VBUS voltage");
> 
> VBUS_VOLTAGE is an optional register. This is not an error. Besides, 
> the
> message doesn't match the event and is useless.
> 
>> +		else
>> +			tcpm_log(tcpci->port, "Invalid VBUS voltage %d", ret);
> 
> Displaying a raw number without context is not very useful.
> 'ret' is the voltage in multiples of 25mV. Besides, the error is that a 
> low
> or high voltage was detected. That doesn't mean the voltage is still 
> invalid.
> The error message should reflect that situation. Something like
> 
> 		"VBUS {low, high} detected, VBUS=x.yy V"
> 
> would be much more useful (with VBUS=x.yy being optional).
> 
> Also, please no tcpm log. The tcpci driver needs to implement
> its own logging if that is desired.
> 

Ok I'll clear up the logging.

>> +	}
>> +
>>  	return IRQ_HANDLED;
>>  }
>>  EXPORT_SYMBOL_GPL(tcpci_irq);
>> --
>> 2.17.1
>> 

[PATCH 3/3] usb: typec: tcpm: Clear the fault status register

[Angus Ainslie (Purism)]

If the fault status register doesn't get cleared then
the ptn5110 interrupt gets stuck on. As the fault register gets
set everytime the ptn5110 powers on the interrupt is always stuck.

Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
---
 drivers/usb/typec/tcpm/tcpci.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index c6e0e48b9a2a..17a0ffe818c0 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -507,6 +507,18 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
 			tcpm_log(tcpci->port, "Invalid VBUS voltage %d", ret);
 	}
 
+	if (status & TCPC_ALERT_FAULT) {
+		u16 fault_status;
+
+		tcpci_read16(tcpci, TCPC_FAULT_STATUS, &fault_status);
+
+		tcpm_log(tcpci->port, "FAULT ALERT status 0x%x",
+				fault_status);
+
+		/* clear the fault status */
+		tcpci_write16(tcpci, TCPC_FAULT_STATUS, fault_status);
+	}
+
 	return IRQ_HANDLED;
 }
 EXPORT_SYMBOL_GPL(tcpci_irq);
-- 
2.17.1

Re: [PATCH 3/3] usb: typec: tcpm: Clear the fault status register

[Fabio Estevam]

Hi Angus,

On Mon, May 6, 2019 at 11:10 AM Angus Ainslie (Purism) <angus@akkea.ca> wrote:
>
> If the fault status register doesn't get cleared then
> the ptn5110 interrupt gets stuck on. As the fault register gets
> set everytime the ptn5110 powers on the interrupt is always stuck.
>
> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>

Since this is a bug fix, I would suggest adding a Fixes tag and Cc
stable if appropriate.

I would also put this patch as the first one in the series, so that it
can be easily applied to older stable trees.

Re: [PATCH 3/3] usb: typec: tcpm: Clear the fault status register

[Guenter Roeck]

On Mon, May 06, 2019 at 12:11:41PM -0300, Fabio Estevam wrote:
> Hi Angus,
> 
> On Mon, May 6, 2019 at 11:10 AM Angus Ainslie (Purism) <angus@akkea.ca> wrote:
> >
> > If the fault status register doesn't get cleared then
> > the ptn5110 interrupt gets stuck on. As the fault register gets
> > set everytime the ptn5110 powers on the interrupt is always stuck.
> >
> > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> 
> Since this is a bug fix, I would suggest adding a Fixes tag and Cc
> stable if appropriate.
> 
> I would also put this patch as the first one in the series, so that it
> can be easily applied to older stable trees.

Unfortunately there is an added tcpm_log() ... and I am opposed to exporting
that.

Guenter

Re: [PATCH 3/3] usb: typec: tcpm: Clear the fault status register

[Angus Ainslie]

On 2019-05-06 09:27, Guenter Roeck wrote:
> On Mon, May 06, 2019 at 12:11:41PM -0300, Fabio Estevam wrote:
>> Hi Angus,
>> 
>> On Mon, May 6, 2019 at 11:10 AM Angus Ainslie (Purism) 
>> <angus@akkea.ca> wrote:
>> >
>> > If the fault status register doesn't get cleared then
>> > the ptn5110 interrupt gets stuck on. As the fault register gets
>> > set everytime the ptn5110 powers on the interrupt is always stuck.
>> >
>> > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>> 
>> Since this is a bug fix, I would suggest adding a Fixes tag and Cc
>> stable if appropriate.
>> 
>> I would also put this patch as the first one in the series, so that it
>> can be easily applied to older stable trees.
> 
> Unfortunately there is an added tcpm_log() ... and I am opposed to 
> exporting
> that.
> 

Ok I'll fix them both up for v2.

> Guenter