The ISRs of the tps25750 and tps6598x do not handle generated events properly under all circumstances.
The tps6598x ISR does not read all bits of the INT_EVENTX registers in newer devices (tps65987 and tps65988, which have 11-byte interrupt registers), leaving events signaled with bits above 64 unattended. Moreover, these events are not cleared, leaving the interrupt enabled.
The tps25750 reads all bits of the INT_EVENT1 register, but the event checking is not right because the same event is checked in two different regions of the same register by means of an OR operation.
This series aims to fix both issues by reading all bits of the INT_EVENTX registers, and limiting the event checking to the region where the supported events are defined (currently they are limited to the first 64 bits of the registers, as the are defined as BIT_ULL()).
In order to identify the interrupt register size, a simple mechanism has been added to read the version register and select the right size accordingly. This mechanism has been tested with a TPS65987DDHRSHR (DH variant) which returned the expected value (0xF7).
If the need for events above the first 64 bits of the INT_EVENTX registers arises, a different mechanism might be required. But for the current needs, all definitions can be left as they are.
When at it, an explicit device_get_match_data() (which is already contained in i2c_get_match_data()) has been removed.
Signed-off-by: Javier Carrasco javier.carrasco@wolfvision.net --- Changes in v3: - Remove explicit usage of device_get_match_data() (new patch). - Fix series versioning (last resend should have been v2). - Keep 64 bit reads in the interrupt handler for tps65981/2/6 by reading the version register. - Link to v2: https://lore.kernel.org/r/20240328-tps6598x_fix_event_handling-v1-0-502721ff...
Changes in v2: - Add Cc tag for 'stable' (fixes). - Link to v1: https://lore.kernel.org/r/20240325-tps6598x_fix_event_handling-v1-0-633fe0c8...
--- Javier Carrasco (3): usb: typec: tipd: fix event checking for tps25750 usb: typec: tipd: fix event checking for tps6598x usb: typec: tipd: rely on i2c_get_match_data()
drivers/usb/typec/tipd/core.c | 56 +++++++++++++++++++++++++-------------- drivers/usb/typec/tipd/tps6598x.h | 11 ++++++++ 2 files changed, 47 insertions(+), 20 deletions(-) --- base-commit: 4cece764965020c22cff7665b18a012006359095 change-id: 20240328-tps6598x_fix_event_handling-3398d3d82f85
Best regards,
In its current form, the interrupt service routine of the tps25750 checks the event flags in the lowest 64 bits of the interrupt event register (event[0]), but also in the upper part (event[1]).
Given that all flags are defined as BIT() or BIT_ULL(), they are restricted to the first 64 bits of the INT_EVENT1 register. Including the upper part of the register can lead to false positives e.g. if the event 64 bits above the one being checked is set, but the one being checked is not.
Restrict the flag checking to the first 64 bits of the INT_EVENT1 register.
Fixes: 7e7a3c815d22 ("USB: typec: tps6598x: Add TPS25750 support") Cc: stable@vger.kernel.org Acked-by: Heikki Krogerus heikki.krogerus@linux.intel.com Signed-off-by: Javier Carrasco javier.carrasco@wolfvision.net --- drivers/usb/typec/tipd/core.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c index 0717cfcd9f8c..7c2f01344860 100644 --- a/drivers/usb/typec/tipd/core.c +++ b/drivers/usb/typec/tipd/core.c @@ -604,11 +604,11 @@ static irqreturn_t tps25750_interrupt(int irq, void *data) if (!tps6598x_read_status(tps, &status)) goto err_clear_ints;
- if ((event[0] | event[1]) & TPS_REG_INT_POWER_STATUS_UPDATE) + if (event[0] & TPS_REG_INT_POWER_STATUS_UPDATE) if (!tps6598x_read_power_status(tps)) goto err_clear_ints;
- if ((event[0] | event[1]) & TPS_REG_INT_DATA_STATUS_UPDATE) + if (event[0] & TPS_REG_INT_DATA_STATUS_UPDATE) if (!tps6598x_read_data_status(tps)) goto err_clear_ints;
@@ -617,7 +617,7 @@ static irqreturn_t tps25750_interrupt(int irq, void *data) * a plug event. Therefore, we need to check * for pr/dr status change to set TypeC dr/pr accordingly. */ - if ((event[0] | event[1]) & TPS_REG_INT_PLUG_EVENT || + if (event[0] & TPS_REG_INT_PLUG_EVENT || tps6598x_has_role_changed(tps, status)) tps6598x_handle_plug_event(tps, status);
The current interrupt service routine of the tps6598x only reads the first 64 bits of the INT_EVENT1 and INT_EVENT2 registers, which means that any event above that range will be ignored, leaving interrupts unattended. Moreover, those events will not be cleared, and the device will keep the interrupt enabled.
This issue has been observed while attempting to load patches, and the 'ReadyForPatch' field (bit 81) of INT_EVENT1 was set.
Given that older versions of the tps6598x (1, 2 and 6) provide 8-byte registers, a mechanism based on the upper byte of the version register (0x0F) has been included. The manufacturer has confirmed [1] that this byte is always 0 for older versions, and either 0xF7 (DH parts) or 0xF9 (DK parts) is returned in newer versions (7 and 8).
Read the complete INT_EVENT registers to handle all interrupts generated by the device and account for the hardware version to select the register size.
Link: https://e2e.ti.com/support/power-management-group/power-management/f/power-m... [1] Fixes: 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers") Cc: stable@vger.kernel.org Signed-off-by: Javier Carrasco javier.carrasco@wolfvision.net --- drivers/usb/typec/tipd/core.c | 45 ++++++++++++++++++++++++++++----------- drivers/usb/typec/tipd/tps6598x.h | 11 ++++++++++ 2 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c index 7c2f01344860..191f86da283d 100644 --- a/drivers/usb/typec/tipd/core.c +++ b/drivers/usb/typec/tipd/core.c @@ -28,6 +28,7 @@ #define TPS_REG_MODE 0x03 #define TPS_REG_CMD1 0x08 #define TPS_REG_DATA1 0x09 +#define TPS_REG_VERSION 0x0F #define TPS_REG_INT_EVENT1 0x14 #define TPS_REG_INT_EVENT2 0x15 #define TPS_REG_INT_MASK1 0x16 @@ -636,49 +637,67 @@ static irqreturn_t tps25750_interrupt(int irq, void *data)
static irqreturn_t tps6598x_interrupt(int irq, void *data) { + int intev_len = TPS_65981_2_6_INTEVENT_LEN; struct tps6598x *tps = data; - u64 event1 = 0; - u64 event2 = 0; + u64 event1[2] = { }; + u64 event2[2] = { }; + u32 version; u32 status; int ret;
mutex_lock(&tps->lock);
- ret = tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event1); - ret |= tps6598x_read64(tps, TPS_REG_INT_EVENT2, &event2); + ret = tps6598x_read32(tps, TPS_REG_VERSION, &version); + if (ret) + dev_warn(tps->dev, "%s: failed to read version (%d)\n", + __func__, ret); + + if (TPS_VERSION_HW_VERSION(version) == TPS_VERSION_HW_65987_8_DH || + TPS_VERSION_HW_VERSION(version) == TPS_VERSION_HW_65987_8_DK) + intev_len = TPS_65987_8_INTEVENT_LEN; + + ret = tps6598x_block_read(tps, TPS_REG_INT_EVENT1, event1, intev_len); + + ret = tps6598x_block_read(tps, TPS_REG_INT_EVENT1, event1, intev_len); if (ret) { - dev_err(tps->dev, "%s: failed to read events\n", __func__); + dev_err(tps->dev, "%s: failed to read event1\n", __func__); goto err_unlock; } - trace_tps6598x_irq(event1, event2); + ret = tps6598x_block_read(tps, TPS_REG_INT_EVENT2, event2, intev_len); + if (ret) { + dev_err(tps->dev, "%s: failed to read event2\n", __func__); + goto err_unlock; + } + trace_tps6598x_irq(event1[0], event2[0]);
- if (!(event1 | event2)) + if (!(event1[0] | event1[1] | event2[0] | event2[1])) goto err_unlock;
if (!tps6598x_read_status(tps, &status)) goto err_clear_ints;
- if ((event1 | event2) & TPS_REG_INT_POWER_STATUS_UPDATE) + if ((event1[0] | event2[0]) & TPS_REG_INT_POWER_STATUS_UPDATE) if (!tps6598x_read_power_status(tps)) goto err_clear_ints;
- if ((event1 | event2) & TPS_REG_INT_DATA_STATUS_UPDATE) + if ((event1[0] | event2[0]) & TPS_REG_INT_DATA_STATUS_UPDATE) if (!tps6598x_read_data_status(tps)) goto err_clear_ints;
/* Handle plug insert or removal */ - if ((event1 | event2) & TPS_REG_INT_PLUG_EVENT) + if ((event1[0] | event2[0]) & TPS_REG_INT_PLUG_EVENT) tps6598x_handle_plug_event(tps, status);
err_clear_ints: - tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event1); - tps6598x_write64(tps, TPS_REG_INT_CLEAR2, event2); + tps6598x_block_write(tps, TPS_REG_INT_CLEAR1, event1, intev_len); + tps6598x_block_write(tps, TPS_REG_INT_CLEAR2, event2, intev_len);
err_unlock: mutex_unlock(&tps->lock);
- if (event1 | event2) + if (event1[0] | event1[1] | event2[0] | event2[1]) return IRQ_HANDLED; + return IRQ_NONE; }
diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h index 89b24519463a..9b23e9017452 100644 --- a/drivers/usb/typec/tipd/tps6598x.h +++ b/drivers/usb/typec/tipd/tps6598x.h @@ -253,4 +253,15 @@ #define TPS_PTCC_DEV 2 #define TPS_PTCC_APP 3
+/* Version Register */ +#define TPS_VERSION_HW_VERSION_MASK GENMASK(31, 24) +#define TPS_VERSION_HW_VERSION(x) TPS_FIELD_GET(TPS_VERSION_HW_VERSION_MASK, (x)) +#define TPS_VERSION_HW_65981_2_6 0x00 +#define TPS_VERSION_HW_65987_8_DH 0xF7 +#define TPS_VERSION_HW_65987_8_DK 0xF9 + +/* Int Event Register length */ +#define TPS_65981_2_6_INTEVENT_LEN 8 +#define TPS_65987_8_INTEVENT_LEN 11 + #endif /* __TPS6598X_H__ */
linux-stable-mirror@lists.linaro.org
-
Javier Carrasco