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 <[email protected]>
---
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-502721ff705b@wolfvision.net
Changes in v2:
- Add Cc tag for 'stable' (fixes).
- Link to v1: https://lore.kernel.org/r/20240325-tps6598x_fix_event_handling-v1-0-633fe0c87cc8@wolfvision.net/
---
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,
--
Javier Carrasco <[email protected]>
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-management-forum/1346521/tps65987d-register-command-to-distinguish-between-tps6591-2-6-and-tps65987-8 [1]
Fixes: 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")
Cc: [email protected]
Signed-off-by: Javier Carrasco <[email protected]>
---
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__ */
--
2.40.1
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: [email protected]
Acked-by: Heikki Krogerus <[email protected]>
Signed-off-by: Javier Carrasco <[email protected]>
---
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);
--
2.40.1
The first thing i2c_get_match_data() does is calling
device_get_match_data(), which already checks if there is a fwnode.
Remove explicit usage of device_get_match_data() as it is already
included in i2c_get_match_data().
Signed-off-by: Javier Carrasco <[email protected]>
---
drivers/usb/typec/tipd/core.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 191f86da283d..ad76dbd20e65 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -1365,10 +1365,7 @@ static int tps6598x_probe(struct i2c_client *client)
TPS_REG_INT_PLUG_EVENT;
}
- if (dev_fwnode(tps->dev))
- tps->data = device_get_match_data(tps->dev);
- else
- tps->data = i2c_get_match_data(client);
+ tps->data = i2c_get_match_data(client);
if (!tps->data)
return -EINVAL;
--
2.40.1
On Mon, Apr 29, 2024 at 03:35:59PM +0200, Javier Carrasco wrote:
> The first thing i2c_get_match_data() does is calling
> device_get_match_data(), which already checks if there is a fwnode.
>
> Remove explicit usage of device_get_match_data() as it is already
> included in i2c_get_match_data().
>
> Signed-off-by: Javier Carrasco <[email protected]>
> ---
> drivers/usb/typec/tipd/core.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On Mon, Apr 29, 2024 at 03:35:59PM +0200, Javier Carrasco wrote:
> The first thing i2c_get_match_data() does is calling
> device_get_match_data(), which already checks if there is a fwnode.
>
> Remove explicit usage of device_get_match_data() as it is already
> included in i2c_get_match_data().
>
> Signed-off-by: Javier Carrasco <[email protected]>
Reviewed-by: Heikki Krogerus <[email protected]>
> ---
> drivers/usb/typec/tipd/core.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 191f86da283d..ad76dbd20e65 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -1365,10 +1365,7 @@ static int tps6598x_probe(struct i2c_client *client)
> TPS_REG_INT_PLUG_EVENT;
> }
>
> - if (dev_fwnode(tps->dev))
> - tps->data = device_get_match_data(tps->dev);
> - else
> - tps->data = i2c_get_match_data(client);
> + tps->data = i2c_get_match_data(client);
> if (!tps->data)
> return -EINVAL;
>
>
> --
> 2.40.1
--
heikki