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

[Guenter Roeck <linux@roeck-us.net>]

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