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@xxxxxxxx>
> > >>>>---
> > >>>>Â 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