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

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

[Angus Ainslie (Purism)]

A high number of interrupts were being generated by the tcpci driver.
This patch outputs debugging info and clears the interrupt condition.

Changes since v1:

Dropped logging to tcpm.
Dropped get_voltage patch.

Angus Ainslie (Purism) (1):
  usb: typec: tcpci: Clear the fault status register

 drivers/usb/typec/tcpm/tcpci.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

-- 
2.17.1

[PATCH v2 1/1] usb: typec: tcpci: 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.

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

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index c1f7073a56de..a5746657b190 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -463,6 +463,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_FAULT) {
+		u16 fault_status;
+
+		tcpci_read16(tcpci, TCPC_FAULT_STATUS, &fault_status);
+
+		dev_warn(tcpci->dev, "FAULT ALERT status 0x%x\n", 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 v2 1/1] usb: typec: tcpci: Clear the fault status register

[Guenter Roeck]

On 5/7/19 5:27 PM, Angus Ainslie (Purism) 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.
> 
> Fixes: fault status register stuck

That is not how Fixes: tags are supposed to work. This should probably be

Fixes: 74e656d6b0551 ("staging: typec: Type-C Port Controller Interface driver (tcpci)")

Otherwise

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> ---
>   drivers/usb/typec/tcpm/tcpci.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index c1f7073a56de..a5746657b190 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -463,6 +463,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_FAULT) {
> +		u16 fault_status;
> +
> +		tcpci_read16(tcpci, TCPC_FAULT_STATUS, &fault_status);
> +
> +		dev_warn(tcpci->dev, "FAULT ALERT status 0x%x\n", fault_status);
> +
> +		/* clear the fault status */
> +		tcpci_write16(tcpci, TCPC_FAULT_STATUS, fault_status);
> +	}
> +
>   	return IRQ_HANDLED;
>   }
>   EXPORT_SYMBOL_GPL(tcpci_irq);
> 

Re: [PATCH v2 1/1] usb: typec: tcpci: Clear the fault status register

[Guenter Roeck]

On 5/7/19 5:27 PM, Angus Ainslie (Purism) 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.
> 
> Fixes: fault status register stuck
> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> ---
>   drivers/usb/typec/tcpm/tcpci.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index c1f7073a56de..a5746657b190 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -463,6 +463,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_FAULT) {

Wait - the driver doesn't set TCPC_ALERT_FAULT in the alert mask
register. How can the chip report it if fault alerts are not enabled ?
What am I missing here ?

Thanks,
Guenter

> +		u16 fault_status;
> +
> +		tcpci_read16(tcpci, TCPC_FAULT_STATUS, &fault_status);
> +
> +		dev_warn(tcpci->dev, "FAULT ALERT status 0x%x\n", fault_status);
> +
> +		/* clear the fault status */
> +		tcpci_write16(tcpci, TCPC_FAULT_STATUS, fault_status);
> +	}
> +
>   	return IRQ_HANDLED;
>   }
>   EXPORT_SYMBOL_GPL(tcpci_irq);
> 

Re: [PATCH v2 1/1] usb: typec: tcpci: Clear the fault status register

[Angus Ainslie]

On 2019-05-07 20:03, Guenter Roeck wrote:
> On 5/7/19 5:27 PM, Angus Ainslie (Purism) 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.
>> 
>> Fixes: fault status register stuck
>> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>> ---
>>   drivers/usb/typec/tcpm/tcpci.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>> 
>> diff --git a/drivers/usb/typec/tcpm/tcpci.c 
>> b/drivers/usb/typec/tcpm/tcpci.c
>> index c1f7073a56de..a5746657b190 100644
>> --- a/drivers/usb/typec/tcpm/tcpci.c
>> +++ b/drivers/usb/typec/tcpm/tcpci.c
>> @@ -463,6 +463,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_FAULT) {
> 
> Wait - the driver doesn't set TCPC_ALERT_FAULT in the alert mask
> register. How can the chip report it if fault alerts are not enabled ?

Well that I didn't check. But I know this code gets executed so 
something must be turning it on.

Also if I don't clear it I get an unlimited number of interrupts.

> What am I missing here ?

Can the power on fault be masked ?

Angus

> 
> Thanks,
> Guenter
> 
>> +		u16 fault_status;
>> +
>> +		tcpci_read16(tcpci, TCPC_FAULT_STATUS, &fault_status);
>> +
>> +		dev_warn(tcpci->dev, "FAULT ALERT status 0x%x\n", fault_status);
>> +
>> +		/* clear the fault status */
>> +		tcpci_write16(tcpci, TCPC_FAULT_STATUS, fault_status);
>> +	}
>> +
>>   	return IRQ_HANDLED;
>>   }
>>   EXPORT_SYMBOL_GPL(tcpci_irq);
>> 

Re: [PATCH v2 1/1] usb: typec: tcpci: Clear the fault status register

[Guenter Roeck]

On 5/7/19 7:49 PM, Angus Ainslie wrote:
> On 2019-05-07 20:03, Guenter Roeck wrote:
>> On 5/7/19 5:27 PM, Angus Ainslie (Purism) 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.
>>>
>>> Fixes: fault status register stuck
>>> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>>> ---
>>>   drivers/usb/typec/tcpm/tcpci.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
>>> index c1f7073a56de..a5746657b190 100644
>>> --- a/drivers/usb/typec/tcpm/tcpci.c
>>> +++ b/drivers/usb/typec/tcpm/tcpci.c
>>> @@ -463,6 +463,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_FAULT) {
>>
>> Wait - the driver doesn't set TCPC_ALERT_FAULT in the alert mask
>> register. How can the chip report it if fault alerts are not enabled ?
> 
> Well that I didn't check. But I know this code gets executed so something must be turning it on.
> 
> Also if I don't clear it I get an unlimited number of interrupts.
> 
>> What am I missing here ?
> 
> Can the power on fault be masked ?
> 

There is a TCPC_ALERT_FAULT mask bit, so I would think so.
Can you dump register contents in the irq function and at the end of
tcpci_init() ?

Thanks,
Guenter

> Angus
> 
>>
>> Thanks,
>> Guenter
>>
>>> +        u16 fault_status;
>>> +
>>> +        tcpci_read16(tcpci, TCPC_FAULT_STATUS, &fault_status);
>>> +
>>> +        dev_warn(tcpci->dev, "FAULT ALERT status 0x%x\n", fault_status);
>>> +
>>> +        /* clear the fault status */
>>> +        tcpci_write16(tcpci, TCPC_FAULT_STATUS, fault_status);
>>> +    }
>>> +
>>>       return IRQ_HANDLED;
>>>   }
>>>   EXPORT_SYMBOL_GPL(tcpci_irq);
>>>
> 
> 

Re: [PATCH v2 1/1] usb: typec: tcpci: Clear the fault status register

[Greg Kroah-Hartman]

On Tue, May 07, 2019 at 05:49:14PM -0700, Guenter Roeck wrote:
> On 5/7/19 5:27 PM, Angus Ainslie (Purism) 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.
> > 
> > Fixes: fault status register stuck
> 
> That is not how Fixes: tags are supposed to work. This should probably be
> 
> Fixes: 74e656d6b0551 ("staging: typec: Type-C Port Controller Interface driver (tcpci)")

Nit:
Fixes: 74e656d6b055 ("staging: typec: Type-C Port Controller Interface driver (tcpci)")

You only need 12 characters of the sha1 :)

Re: [PATCH v2 1/1] usb: typec: tcpci: Clear the fault status register

[Angus Ainslie]

Hi Guenter

On 2019-05-07 23:18, Guenter Roeck wrote:
> On 5/7/19 7:49 PM, Angus Ainslie wrote:
>> On 2019-05-07 20:03, Guenter Roeck wrote:
>>> On 5/7/19 5:27 PM, Angus Ainslie (Purism) 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.
>>>> 
>>>> Fixes: fault status register stuck
>>>> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>>>> ---
>>>>   drivers/usb/typec/tcpm/tcpci.c | 11 +++++++++++
>>>>   1 file changed, 11 insertions(+)
>>>> 
>>>> diff --git a/drivers/usb/typec/tcpm/tcpci.c 
>>>> b/drivers/usb/typec/tcpm/tcpci.c
>>>> index c1f7073a56de..a5746657b190 100644
>>>> --- a/drivers/usb/typec/tcpm/tcpci.c
>>>> +++ b/drivers/usb/typec/tcpm/tcpci.c
>>>> @@ -463,6 +463,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_FAULT) {
>>> 
>>> Wait - the driver doesn't set TCPC_ALERT_FAULT in the alert mask
>>> register. How can the chip report it if fault alerts are not enabled 
>>> ?
>> 
>> Well that I didn't check. But I know this code gets executed so 
>> something must be turning it on.
>> 
>> Also if I don't clear it I get an unlimited number of interrupts.
>> 
>>> What am I missing here ?
>> 
>> Can the power on fault be masked ?
>> 
> 
> There is a TCPC_ALERT_FAULT mask bit, so I would think so.
> Can you dump register contents in the irq function and at the end of
> tcpci_init() ?
> 

Ok so this seems to be related to imx8mq errata e7805:

I2C: When the I2C clock speed is configured for 400 kHz, the SCL low 
period violates the I2C spec of
1.3 uS min

The work around suggested by NXP is to set the clock to 384 kHz so that 
is what I did and this is the output:

[    4.091512] device: 'tcpm-source-psy-0-0052': device_add
[    4.091581] PM: Adding info for No Bus:tcpm-source-psy-0-0052
[    4.091596] device: 'tcpm-source-psy-0-0052': dev_uevent: class 
uevent() returned -11
[    4.094774] tcpci 0-0052: ALERT MASK 0x7f
[    4.107869] driver: 'tcpci': driver_bound: bound to device '0-0052'
[    4.107935] bus: 'i2c': really_probe: bound device 0-0052 to driver 
tcpci
[    4.110994] tcpci 0-0052: ALERT MASK 0x7f
[    4.115511] tcpci 0-0052: FAULT ALERT status 0x80
[    4.126332] tcpci 0-0052: ALERT MASK 0x7f
[    4.130784] tcpci 0-0052: FAULT ALERT status 0x0

The first "ALERT MASK" is in the init function immediately after setting

         reg = TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_FAILED |
                 TCPC_ALERT_TX_DISCARDED | TCPC_ALERT_RX_STATUS |
                 TCPC_ALERT_RX_HARD_RST | TCPC_ALERT_CC_STATUS;
         if (tcpci->controls_vbus)
                 reg |= TCPC_ALERT_POWER_STATUS;
         ret = tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);


So it looks like the register is correct but the fault interrupt still 
fires. At 200 kHz I get the following output.

[    4.136845] device: 'tcpm-source-psy-0-0052': device_add
[    4.136943] PM: Adding info for No Bus:tcpm-source-psy-0-0052
[    4.136966] device: 'tcpm-source-psy-0-0052': dev_uevent: class 
uevent() returned -11
[    4.178510] tcpci 0-0052: ALERT MASK 0x7f
[    4.217197] driver: 'tcpci': driver_bound: bound to device '0-0052'
[    4.217371] bus: 'i2c': really_probe: bound device 0-0052 to driver 
tcpci

So this is what is expected no fault interrupt.

Maybe errata e7805 needs an update.

Sorry for the noise.

Cheers
Angus


> Thanks,
> Guenter
> 
>> Angus
>> 
>>> 
>>> Thanks,
>>> Guenter
>>> 
>>>> +        u16 fault_status;
>>>> +
>>>> +        tcpci_read16(tcpci, TCPC_FAULT_STATUS, &fault_status);
>>>> +
>>>> +        dev_warn(tcpci->dev, "FAULT ALERT status 0x%x\n", 
>>>> fault_status);
>>>> +
>>>> +        /* clear the fault status */
>>>> +        tcpci_write16(tcpci, TCPC_FAULT_STATUS, fault_status);
>>>> +    }
>>>> +
>>>>       return IRQ_HANDLED;
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(tcpci_irq);
>>>> 
>> 
>> 

Re: [PATCH v2 1/1] usb: typec: tcpci: Clear the fault status register

[Guenter Roeck]

On Wed, May 08, 2019 at 07:48:43AM -0600, Angus Ainslie wrote:
> Hi Guenter
> 
> On 2019-05-07 23:18, Guenter Roeck wrote:
> >On 5/7/19 7:49 PM, Angus Ainslie wrote:
> >>On 2019-05-07 20:03, Guenter Roeck wrote:
> >>>On 5/7/19 5:27 PM, Angus Ainslie (Purism) 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.
> >>>>
> >>>>Fixes: fault status register stuck
> >>>>Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> >>>>---
> >>>>  drivers/usb/typec/tcpm/tcpci.c | 11 +++++++++++
> >>>>  1 file changed, 11 insertions(+)
> >>>>
> >>>>diff --git a/drivers/usb/typec/tcpm/tcpci.c
> >>>>b/drivers/usb/typec/tcpm/tcpci.c
> >>>>index c1f7073a56de..a5746657b190 100644
> >>>>--- a/drivers/usb/typec/tcpm/tcpci.c
> >>>>+++ b/drivers/usb/typec/tcpm/tcpci.c
> >>>>@@ -463,6 +463,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_FAULT) {
> >>>
> >>>Wait - the driver doesn't set TCPC_ALERT_FAULT in the alert mask
> >>>register. How can the chip report it if fault alerts are not enabled ?
> >>
> >>Well that I didn't check. But I know this code gets executed so
> >>something must be turning it on.
> >>
> >>Also if I don't clear it I get an unlimited number of interrupts.
> >>
> >>>What am I missing here ?
> >>
> >>Can the power on fault be masked ?
> >>
> >
> >There is a TCPC_ALERT_FAULT mask bit, so I would think so.
> >Can you dump register contents in the irq function and at the end of
> >tcpci_init() ?
> >
> 
> Ok so this seems to be related to imx8mq errata e7805:
> 
> I2C: When the I2C clock speed is configured for 400 kHz, the SCL low period
> violates the I2C spec of
> 1.3 uS min
> 
> The work around suggested by NXP is to set the clock to 384 kHz so that is
> what I did and this is the output:
> 
> [    4.091512] device: 'tcpm-source-psy-0-0052': device_add
> [    4.091581] PM: Adding info for No Bus:tcpm-source-psy-0-0052
> [    4.091596] device: 'tcpm-source-psy-0-0052': dev_uevent: class uevent()
> returned -11
> [    4.094774] tcpci 0-0052: ALERT MASK 0x7f
> [    4.107869] driver: 'tcpci': driver_bound: bound to device '0-0052'
> [    4.107935] bus: 'i2c': really_probe: bound device 0-0052 to driver tcpci
> [    4.110994] tcpci 0-0052: ALERT MASK 0x7f
> [    4.115511] tcpci 0-0052: FAULT ALERT status 0x80
> [    4.126332] tcpci 0-0052: ALERT MASK 0x7f
> [    4.130784] tcpci 0-0052: FAULT ALERT status 0x0
> 
> The first "ALERT MASK" is in the init function immediately after setting
> 
>         reg = TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_FAILED |
>                 TCPC_ALERT_TX_DISCARDED | TCPC_ALERT_RX_STATUS |
>                 TCPC_ALERT_RX_HARD_RST | TCPC_ALERT_CC_STATUS;
>         if (tcpci->controls_vbus)
>                 reg |= TCPC_ALERT_POWER_STATUS;
>         ret = tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);
> 
> 
> So it looks like the register is correct but the fault interrupt still
> fires. At 200 kHz I get the following output.
> 
> [    4.136845] device: 'tcpm-source-psy-0-0052': device_add
> [    4.136943] PM: Adding info for No Bus:tcpm-source-psy-0-0052
> [    4.136966] device: 'tcpm-source-psy-0-0052': dev_uevent: class uevent()
> returned -11
> [    4.178510] tcpci 0-0052: ALERT MASK 0x7f
> [    4.217197] driver: 'tcpci': driver_bound: bound to device '0-0052'
> [    4.217371] bus: 'i2c': really_probe: bound device 0-0052 to driver tcpci
> 
> So this is what is expected no fault interrupt.
> 
> Maybe errata e7805 needs an update.
> 
> Sorry for the noise.
> 

Let's not jump to conclusions; I don't think this is noise. It is more
likely that the i2c problem uncovers a race condition in tcpci_init().

In tcpci_init(), we first clear TCPC_ALERT by writing 0xffff into it.
Subsequently, we set TCPC_ALERT_MASK. I suspect what may happen is 
that the chip has FAULT_ALERT enabled, and that a fault was logged.
We don't clear the FAULT_STATUS register in tcpci_init(), thus
FAULT_ALERT is immediately set again, before we clear the FAULT_ALERT
mask bit.

The standard says that the alert pin shall not be set if the respective
interrupt is masked, but maybe the chip doesn't follow that. Either case,
the standard does say that masked alerts are still reported in the status
registers, so it is not surprising that the fault status is reported.

What we should probably do in tcpci_init() is to change the register
initialization order, and to clear the fault status register.

- Set TCPC_ALERT_MASK
- Set FAULT_STATUS_MASK (to 0)
- Clear TCPC_FAULT_STATUS
- Clear TCPC_ALERT

I suspect that will fix your problem.

Another question is if TCPC_ALERT_FAULT (together with appropriate
FAULT_STATUS_MASK bits) should be enabled, and if faults should be
logged. But that would be a separate patch or patch series.

Thanks,
Guenter

Re: [PATCH v2 1/1] usb: typec: tcpci: Clear the fault status register

[Angus Ainslie]

On 2019-05-08 10:22, Guenter Roeck wrote:
> On Wed, May 08, 2019 at 07:48:43AM -0600, Angus Ainslie wrote:
>> Hi Guenter
>> 
>> On 2019-05-07 23:18, Guenter Roeck wrote:
>> >On 5/7/19 7:49 PM, Angus Ainslie wrote:
>> >>On 2019-05-07 20:03, Guenter Roeck wrote:
>> >>>On 5/7/19 5:27 PM, Angus Ainslie (Purism) 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.
>> >>>>
>> >>>>Fixes: fault status register stuck
>> >>>>Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>> >>>>---
>> >>>>  drivers/usb/typec/tcpm/tcpci.c | 11 +++++++++++
>> >>>>  1 file changed, 11 insertions(+)
>> >>>>
>> >>>>diff --git a/drivers/usb/typec/tcpm/tcpci.c
>> >>>>b/drivers/usb/typec/tcpm/tcpci.c
>> >>>>index c1f7073a56de..a5746657b190 100644
>> >>>>--- a/drivers/usb/typec/tcpm/tcpci.c
>> >>>>+++ b/drivers/usb/typec/tcpm/tcpci.c
>> >>>>@@ -463,6 +463,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_FAULT) {
>> >>>
>> >>>Wait - the driver doesn't set TCPC_ALERT_FAULT in the alert mask
>> >>>register. How can the chip report it if fault alerts are not enabled ?
>> >>
>> >>Well that I didn't check. But I know this code gets executed so
>> >>something must be turning it on.
>> >>
>> >>Also if I don't clear it I get an unlimited number of interrupts.
>> >>
>> >>>What am I missing here ?
>> >>
>> >>Can the power on fault be masked ?
>> >>
>> >
>> >There is a TCPC_ALERT_FAULT mask bit, so I would think so.
>> >Can you dump register contents in the irq function and at the end of
>> >tcpci_init() ?
>> >
>> 
>> Ok so this seems to be related to imx8mq errata e7805:
>> 
>> I2C: When the I2C clock speed is configured for 400 kHz, the SCL low 
>> period
>> violates the I2C spec of
>> 1.3 uS min
>> 
>> The work around suggested by NXP is to set the clock to 384 kHz so 
>> that is
>> what I did and this is the output:
>> 
>> [    4.091512] device: 'tcpm-source-psy-0-0052': device_add
>> [    4.091581] PM: Adding info for No Bus:tcpm-source-psy-0-0052
>> [    4.091596] device: 'tcpm-source-psy-0-0052': dev_uevent: class 
>> uevent()
>> returned -11
>> [    4.094774] tcpci 0-0052: ALERT MASK 0x7f
>> [    4.107869] driver: 'tcpci': driver_bound: bound to device '0-0052'
>> [    4.107935] bus: 'i2c': really_probe: bound device 0-0052 to driver 
>> tcpci
>> [    4.110994] tcpci 0-0052: ALERT MASK 0x7f
>> [    4.115511] tcpci 0-0052: FAULT ALERT status 0x80
>> [    4.126332] tcpci 0-0052: ALERT MASK 0x7f
>> [    4.130784] tcpci 0-0052: FAULT ALERT status 0x0
>> 
>> The first "ALERT MASK" is in the init function immediately after 
>> setting
>> 
>>         reg = TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_FAILED |
>>                 TCPC_ALERT_TX_DISCARDED | TCPC_ALERT_RX_STATUS |
>>                 TCPC_ALERT_RX_HARD_RST | TCPC_ALERT_CC_STATUS;
>>         if (tcpci->controls_vbus)
>>                 reg |= TCPC_ALERT_POWER_STATUS;
>>         ret = tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);
>> 
>> 
>> So it looks like the register is correct but the fault interrupt still
>> fires. At 200 kHz I get the following output.
>> 
>> [    4.136845] device: 'tcpm-source-psy-0-0052': device_add
>> [    4.136943] PM: Adding info for No Bus:tcpm-source-psy-0-0052
>> [    4.136966] device: 'tcpm-source-psy-0-0052': dev_uevent: class 
>> uevent()
>> returned -11
>> [    4.178510] tcpci 0-0052: ALERT MASK 0x7f
>> [    4.217197] driver: 'tcpci': driver_bound: bound to device '0-0052'
>> [    4.217371] bus: 'i2c': really_probe: bound device 0-0052 to driver 
>> tcpci
>> 
>> So this is what is expected no fault interrupt.
>> 
>> Maybe errata e7805 needs an update.
>> 
>> Sorry for the noise.
>> 
> 
> Let's not jump to conclusions; I don't think this is noise. It is more
> likely that the i2c problem uncovers a race condition in tcpci_init().
> 
> In tcpci_init(), we first clear TCPC_ALERT by writing 0xffff into it.
> Subsequently, we set TCPC_ALERT_MASK. I suspect what may happen is
> that the chip has FAULT_ALERT enabled, and that a fault was logged.
> We don't clear the FAULT_STATUS register in tcpci_init(), thus
> FAULT_ALERT is immediately set again, before we clear the FAULT_ALERT
> mask bit.
> 

Ok but wouldn't slowing down the bus speed make this more likely to 
happen than less ?

> The standard says that the alert pin shall not be set if the respective
> interrupt is masked, but maybe the chip doesn't follow that. Either 
> case,
> the standard does say that masked alerts are still reported in the 
> status
> registers, so it is not surprising that the fault status is reported.
> 
> What we should probably do in tcpci_init() is to change the register
> initialization order, and to clear the fault status register.
> 
> - Set TCPC_ALERT_MASK
> - Set FAULT_STATUS_MASK (to 0)
> - Clear TCPC_FAULT_STATUS
> - Clear TCPC_ALERT
> 
> I suspect that will fix your problem.
> 

I'll try and get time to give that a shot.

> Another question is if TCPC_ALERT_FAULT (together with appropriate
> FAULT_STATUS_MASK bits) should be enabled, and if faults should be
> logged. But that would be a separate patch or patch series.
> 

I was thinking this too but it also falls into the if I can find time 
category.

Angus

> Thanks,
> Guenter

Re: [PATCH v2 1/1] usb: typec: tcpci: Clear the fault status register

[Guenter Roeck]

On Wed, May 08, 2019 at 10:33:22AM -0600, Angus Ainslie wrote:
> On 2019-05-08 10:22, Guenter Roeck wrote:
> >On Wed, May 08, 2019 at 07:48:43AM -0600, Angus Ainslie wrote:
> >>Hi Guenter
> >>
> >>On 2019-05-07 23:18, Guenter Roeck wrote:
> >>>On 5/7/19 7:49 PM, Angus Ainslie wrote:
> >>>>On 2019-05-07 20:03, Guenter Roeck wrote:
> >>>>>On 5/7/19 5:27 PM, Angus Ainslie (Purism) 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.
> >>>>>>
> >>>>>>Fixes: fault status register stuck
> >>>>>>Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> >>>>>>---
> >>>>>>  drivers/usb/typec/tcpm/tcpci.c | 11 +++++++++++
> >>>>>>  1 file changed, 11 insertions(+)
> >>>>>>
> >>>>>>diff --git a/drivers/usb/typec/tcpm/tcpci.c
> >>>>>>b/drivers/usb/typec/tcpm/tcpci.c
> >>>>>>index c1f7073a56de..a5746657b190 100644
> >>>>>>--- a/drivers/usb/typec/tcpm/tcpci.c
> >>>>>>+++ b/drivers/usb/typec/tcpm/tcpci.c
> >>>>>>@@ -463,6 +463,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_FAULT) {
> >>>>>
> >>>>>Wait - the driver doesn't set TCPC_ALERT_FAULT in the alert mask
> >>>>>register. How can the chip report it if fault alerts are not enabled ?
> >>>>
> >>>>Well that I didn't check. But I know this code gets executed so
> >>>>something must be turning it on.
> >>>>
> >>>>Also if I don't clear it I get an unlimited number of interrupts.
> >>>>
> >>>>>What am I missing here ?
> >>>>
> >>>>Can the power on fault be masked ?
> >>>>
> >>>
> >>>There is a TCPC_ALERT_FAULT mask bit, so I would think so.
> >>>Can you dump register contents in the irq function and at the end of
> >>>tcpci_init() ?
> >>>
> >>
> >>Ok so this seems to be related to imx8mq errata e7805:
> >>
> >>I2C: When the I2C clock speed is configured for 400 kHz, the SCL low
> >>period
> >>violates the I2C spec of
> >>1.3 uS min
> >>
> >>The work around suggested by NXP is to set the clock to 384 kHz so that
> >>is
> >>what I did and this is the output:
> >>
> >>[    4.091512] device: 'tcpm-source-psy-0-0052': device_add
> >>[    4.091581] PM: Adding info for No Bus:tcpm-source-psy-0-0052
> >>[    4.091596] device: 'tcpm-source-psy-0-0052': dev_uevent: class
> >>uevent()
> >>returned -11
> >>[    4.094774] tcpci 0-0052: ALERT MASK 0x7f
> >>[    4.107869] driver: 'tcpci': driver_bound: bound to device '0-0052'
> >>[    4.107935] bus: 'i2c': really_probe: bound device 0-0052 to driver
> >>tcpci
> >>[    4.110994] tcpci 0-0052: ALERT MASK 0x7f
> >>[    4.115511] tcpci 0-0052: FAULT ALERT status 0x80
> >>[    4.126332] tcpci 0-0052: ALERT MASK 0x7f
> >>[    4.130784] tcpci 0-0052: FAULT ALERT status 0x0
> >>
> >>The first "ALERT MASK" is in the init function immediately after setting
> >>
> >>        reg = TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_FAILED |
> >>                TCPC_ALERT_TX_DISCARDED | TCPC_ALERT_RX_STATUS |
> >>                TCPC_ALERT_RX_HARD_RST | TCPC_ALERT_CC_STATUS;
> >>        if (tcpci->controls_vbus)
> >>                reg |= TCPC_ALERT_POWER_STATUS;
> >>        ret = tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);
> >>
> >>
> >>So it looks like the register is correct but the fault interrupt still
> >>fires. At 200 kHz I get the following output.
> >>
> >>[    4.136845] device: 'tcpm-source-psy-0-0052': device_add
> >>[    4.136943] PM: Adding info for No Bus:tcpm-source-psy-0-0052
> >>[    4.136966] device: 'tcpm-source-psy-0-0052': dev_uevent: class
> >>uevent()
> >>returned -11
> >>[    4.178510] tcpci 0-0052: ALERT MASK 0x7f
> >>[    4.217197] driver: 'tcpci': driver_bound: bound to device '0-0052'
> >>[    4.217371] bus: 'i2c': really_probe: bound device 0-0052 to driver
> >>tcpci
> >>
> >>So this is what is expected no fault interrupt.
> >>
> >>Maybe errata e7805 needs an update.
> >>
> >>Sorry for the noise.
> >>
> >
> >Let's not jump to conclusions; I don't think this is noise. It is more
> >likely that the i2c problem uncovers a race condition in tcpci_init().
> >
> >In tcpci_init(), we first clear TCPC_ALERT by writing 0xffff into it.
> >Subsequently, we set TCPC_ALERT_MASK. I suspect what may happen is
> >that the chip has FAULT_ALERT enabled, and that a fault was logged.
> >We don't clear the FAULT_STATUS register in tcpci_init(), thus
> >FAULT_ALERT is immediately set again, before we clear the FAULT_ALERT
> >mask bit.
> >
> 
> Ok but wouldn't slowing down the bus speed make this more likely to happen
> than less ?
> 
No, I don't think so. After all, FAULT_ALERT should be set again immediately
if FAULT_STATUS was not cleared. I think the speed problem by itself may
trigger a fault, and we see the secondary impact of that. Is FAULT_STATUS
bit 0 (I2C Interface Error Interrupt) set, by any chance ?

Plus, you don't see a fault in the first place with the lower speed, or
did I miss something ?

> >The standard says that the alert pin shall not be set if the respective
> >interrupt is masked, but maybe the chip doesn't follow that. Either case,
> >the standard does say that masked alerts are still reported in the status
> >registers, so it is not surprising that the fault status is reported.
> >
> >What we should probably do in tcpci_init() is to change the register
> >initialization order, and to clear the fault status register.
> >
> >- Set TCPC_ALERT_MASK
> >- Set FAULT_STATUS_MASK (to 0)
> >- Clear TCPC_FAULT_STATUS
> >- Clear TCPC_ALERT
> >
> >I suspect that will fix your problem.
> >
> 
> I'll try and get time to give that a shot.
> 
> >Another question is if TCPC_ALERT_FAULT (together with appropriate
> >FAULT_STATUS_MASK bits) should be enabled, and if faults should be
> >logged. But that would be a separate patch or patch series.
> >
> 
> I was thinking this too but it also falls into the if I can find time
> category.
> 
Seems like we all have the same problem ...

Thanks,
Guenter