Messages in this thread | | | Subject | Re: [PATCH v2 1/1] usb: typec: tcpci: Clear the fault status register | Date | Wed, 08 May 2019 10:33:22 -0600 | From | 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
| |