lkml.org 
[lkml]   [2019]   [May]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/1] usb: typec: tcpci: Clear the fault status register
Date
From
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

\
 
 \ /
  Last update: 2019-05-08 18:34    [W:0.064 / U:2.656 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site