[PATCH v3 0/3] usb: typec: tipd: add TPS66993 support

[PATCH v3 0/3] usb: typec: tipd: add TPS66993 support

[Radhey Shyam Pandey]

This series adds support for the Texas Instruments TPS66993 USB
Type-C / USB PD controller to the tipd driver.

The TPS66993 is used on the AMD Versal AI Edge Gen 2 VEK385
Evaluation Kit as the USB PD DRP controller for the MMI USB
interface, handling CC signaling, connection detection, PD
negotiation, and power/data role swapping. Although the register
interface is largely shared with TPS6598x, the device is not
host-interface compatible and therefore needs its own
"ti,tps66993" compatible.

Changes in v3 series:
- Add Krzysztof and Heikki reviewed-by tag in 1/3 and 2/3 patch.
- Remove the unnecessary if (tps->data->trace_power_status) guard and
  call the trace callback unconditionally, since every tipd_data entry
  always provides it.
 
Changes in v2 series:
- Document the "ti,tps66993" compatible in the existing
  ti,tps6598x binding.
- Introduce a read_power_status() callback in tipd_data so
  variants can override how power status is obtained without
  changing TPS6598x, CD321x, or TPS25750 behavior.
- Add TPS66993 support.

Radhey Shyam Pandey (3):
  dt-bindings: usb: ti,tps6598x: add TPS66993 compatible
  usb: typec: tipd: add read_power_status callback to tipd_data
  usb: typec: tipd: add TPS66993 support

 .../devicetree/bindings/usb/ti,tps6598x.yaml  |  6 +-
 drivers/usb/typec/tipd/core.c                 | 76 +++++++++++++++++--
 2 files changed, 74 insertions(+), 8 deletions(-)


base-commit: 49362394dad7df66c274c867a271394c10ca2bb8
-- 
2.49.1

[PATCH v3 1/3] dt-bindings: usb: ti,tps6598x: add TPS66993 compatible

[Radhey Shyam Pandey]

Add a ti,tps66993 compatible to explicitly identify TPS66993 devices.
The TPS66993 is not host-interface compatible with TPS6598x, so a distinct
compatible is required.

On the AMD/Xilinx VEK385 Evaluation Board, the Texas Instruments TPS66993
acts as the USB Type-C/USB PD DRP controller for the MMI USB interface,
handling CC signaling, connection detection, PD negotiation and power/data
role swapping.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
Changes for v3:
- Add Krzysztof acked-by tag.

Changes for v2:
- Rename the compatible to match the exact device (ti,tps66993) and in
  commit mention the usage of TPS66993 chip on VEK385 Evaluation Board.
---
 Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
index 1745e28b3110..2c589e9e712e 100644
--- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
@@ -4,13 +4,14 @@
 $id: http://devicetree.org/schemas/usb/ti,tps6598x.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Texas Instruments 6598x Type-C Port Switch and Power Delivery controller
+title: Texas Instruments Type-C port switch and USB Power Delivery controllers
 
 maintainers:
   - Bryan O'Donoghue <bryan.odonoghue@linaro.org>
 
 description: |
-  Texas Instruments 6598x Type-C Port Switch and Power Delivery controller
+  Texas Instruments 6598x and 66993 Type-C Port Switch and Power Delivery
+  controller.
 
   A variant of this controller known as Apple CD321x or Apple ACE is also
   present on hardware with Apple SoCs such as the M1.
@@ -19,6 +20,7 @@ properties:
   compatible:
     enum:
       - ti,tps6598x
+      - ti,tps66993
       - apple,cd321x
       - ti,tps25750
 
-- 
2.49.1

[PATCH v3 2/3] usb: typec: tipd: add read_power_status callback to tipd_data

[Radhey Shyam Pandey]

Convert direct tps6598x_read_power_status() calls to use an indirect
read_power_status callback through tipd_data. This allows variants
(e.g. TPS66993) to provide their own power status reading logic while
keeping existing behavior unchanged for TPS6598x, CD321x, and TPS25750.

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
Changes for v3:
- Add Heikki Krogerus reviewed-by tag.

Changes for v2:
- New patch to add TPS66993 driver support.
---
 drivers/usb/typec/tipd/core.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index b6335b36d384..9097b0d40a71 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -159,6 +159,7 @@ struct tipd_data {
 	int (*init)(struct tps6598x *tps);
 	int (*switch_power_state)(struct tps6598x *tps, u8 target_state);
 	bool (*read_data_status)(struct tps6598x *tps);
+	bool (*read_power_status)(struct tps6598x *tps);
 	int (*reset)(struct tps6598x *tps);
 	int (*connect)(struct tps6598x *tps, u32 status);
 };
@@ -897,7 +898,7 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
 		goto err_unlock;
 
 	if (event & APPLE_CD_REG_INT_POWER_STATUS_UPDATE) {
-		if (!tps6598x_read_power_status(tps))
+		if (!tps->data->read_power_status(tps))
 			goto err_unlock;
 		if (TPS_POWER_STATUS_PWROPMODE(tps->pwr_status) == TYPEC_PWR_MODE_PD) {
 			if (tps6598x_read_partner_identity(tps)) {
@@ -952,7 +953,7 @@ static irqreturn_t tps25750_interrupt(int irq, void *data)
 		goto err_clear_ints;
 
 	if (event[0] & TPS_REG_INT_POWER_STATUS_UPDATE)
-		if (!tps6598x_read_power_status(tps))
+		if (!tps->data->read_power_status(tps))
 			goto err_clear_ints;
 
 	if (event[0] & TPS_REG_INT_DATA_STATUS_UPDATE)
@@ -1026,7 +1027,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 		goto err_unlock;
 
 	if ((event1[0] | event2[0]) & TPS_REG_INT_POWER_STATUS_UPDATE)
-		if (!tps6598x_read_power_status(tps))
+		if (!tps->data->read_power_status(tps))
 			goto err_unlock;
 
 	if ((event1[0] | event2[0]) & TPS_REG_INT_DATA_STATUS_UPDATE)
@@ -1839,7 +1840,7 @@ static int tps6598x_probe(struct i2c_client *client)
 
 	if (status & TPS_STATUS_PLUG_PRESENT) {
 		ret = -EINVAL;
-		if (!tps6598x_read_power_status(tps))
+		if (!tps->data->read_power_status(tps))
 			goto err_unregister_port;
 		if (!tps->data->read_data_status(tps))
 			goto err_unregister_port;
@@ -1981,6 +1982,7 @@ static const struct tipd_data cd321x_data = {
 	.trace_status = trace_tps6598x_status,
 	.init = cd321x_init,
 	.read_data_status = cd321x_read_data_status,
+	.read_power_status = tps6598x_read_power_status,
 	.reset = cd321x_reset,
 	.switch_power_state = cd321x_switch_power_state,
 	.connect = cd321x_connect,
@@ -2000,6 +2002,7 @@ static const struct tipd_data tps6598x_data = {
 	.apply_patch = tps6598x_apply_patch,
 	.init = tps6598x_init,
 	.read_data_status = tps6598x_read_data_status,
+	.read_power_status = tps6598x_read_power_status,
 	.reset = tps6598x_reset,
 	.connect = tps6598x_connect,
 };
@@ -2018,6 +2021,7 @@ static const struct tipd_data tps25750_data = {
 	.apply_patch = tps25750_apply_patch,
 	.init = tps25750_init,
 	.read_data_status = tps6598x_read_data_status,
+	.read_power_status = tps6598x_read_power_status,
 	.reset = tps25750_reset,
 	.connect = tps6598x_connect,
 };
-- 
2.49.1

[PATCH v3 3/3] usb: typec: tipd: add TPS66993 support

[Radhey Shyam Pandey]

Derive power status from the STATUS register (0x1A) now that TPS66993
deprecates the Power_Status register (0x3F). Add support for the "APP1"
mode string. TPS66993 controller is configured in polling mode and only
type-c flip orientation feature is supported on AMD Versal AI Edge Gen 2
VEK385 Evaluation Kit.

Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
Changes for v3:
-  Remove the unnecessary if (tps->data->trace_power_status) guard and
   call the trace callback unconditionally, since every tipd_data entry
   always provides it.

Changes for v2:
- New patch to add TPS66993 driver support.
---
 drivers/usb/typec/tipd/core.c | 64 +++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 9097b0d40a71..522f56742aa9 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -130,6 +130,7 @@ enum {
 	TPS_MODE_BIST,
 	TPS_MODE_DISC,
 	TPS_MODE_PTCH,
+	TPS_MODE_APP1,
 };
 
 static const char *const modes[] = {
@@ -138,6 +139,7 @@ static const char *const modes[] = {
 	[TPS_MODE_BIST]	= "BIST",
 	[TPS_MODE_DISC]	= "DISC",
 	[TPS_MODE_PTCH] = "PTCH",
+	[TPS_MODE_APP1]	= "APP1",
 };
 
 /* Unrecognized commands will be replaced with "!CMD" */
@@ -631,6 +633,34 @@ static bool tps6598x_read_power_status(struct tps6598x *tps)
 	return true;
 }
 
+/*
+ * TPS66993 deprecated Power_Status register (0x3F). BC1.2 is not supported
+ * and the remaining bits are redundant with STATUS register (0x1A).
+ * Synthesize pwr_status from the already-read STATUS register.
+ */
+static bool tps66993_read_power_status(struct tps6598x *tps)
+{
+	u16 pwr_status = 0;
+
+	/* Same masks as TPS_POWER_STATUS_CONNECTION() / SOURCESINK() / PWROPMODE() in tps6598x.h */
+	if (tps->status & TPS_STATUS_PLUG_PRESENT)
+		pwr_status |= FIELD_PREP(TPS_POWER_STATUS_CONNECTION_MASK, 1);
+
+	/* SOURCESINK: 1=sink; STATUS.PortRole 1=source, opposite convention */
+	if (!TPS_STATUS_TO_TYPEC_PORTROLE(tps->status))
+		pwr_status |= FIELD_PREP(TPS_POWER_STATUS_SOURCESINK_MASK, 1);
+
+	if (TPS_STATUS_VBUS_STATUS(tps->status) == TPS_STATUS_VBUS_STATUS_PD)
+		pwr_status |= FIELD_PREP(TPS_POWER_STATUS_TYPEC_CURRENT_MASK,
+					 TPS_POWER_STATUS_TYPEC_CURRENT_PD);
+
+	tps->pwr_status = pwr_status;
+
+	tps->data->trace_power_status(pwr_status);
+
+	return true;
+}
+
 static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status)
 {
 	int ret;
@@ -1026,6 +1056,8 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 	if (!tps6598x_read_status(tps, &status))
 		goto err_unlock;
 
+	tps->status = status;
+
 	if ((event1[0] | event2[0]) & TPS_REG_INT_POWER_STATUS_UPDATE)
 		if (!tps->data->read_power_status(tps))
 			goto err_unlock;
@@ -1034,9 +1066,15 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 		if (!tps->data->read_data_status(tps))
 			goto err_unlock;
 
-	/* Handle plug insert or removal */
-	if ((event1[0] | event2[0]) & TPS_REG_INT_PLUG_EVENT)
+	/*
+	 * Refresh power status before connect - needed for TPS66993 which
+	 * synthesizes pwr_status from STATUS and never gets POWER_STATUS_UPDATE.
+	 */
+	if ((event1[0] | event2[0]) & TPS_REG_INT_PLUG_EVENT) {
+		if (!tps->data->read_power_status(tps))
+			goto err_unlock;
 		tps6598x_handle_plug_event(tps, status);
+	}
 
 err_unlock:
 	mutex_unlock(&tps->lock);
@@ -1072,6 +1110,7 @@ static int tps6598x_check_mode(struct tps6598x *tps)
 
 	switch (ret) {
 	case TPS_MODE_APP:
+	case TPS_MODE_APP1:
 	case TPS_MODE_PTCH:
 		return ret;
 	case TPS_MODE_BOOT:
@@ -1813,6 +1852,8 @@ static int tps6598x_probe(struct i2c_client *client)
 		goto err_clear_mask;
 	}
 
+	tps->status = status;
+
 	/*
 	 * This fwnode has a "compatible" property, but is never populated as a
 	 * struct device. Instead we simply parse it to read the properties.
@@ -2007,6 +2048,24 @@ static const struct tipd_data tps6598x_data = {
 	.connect = tps6598x_connect,
 };
 
+static const struct tipd_data tps66993_data = {
+	.irq_handler = tps6598x_interrupt,
+	.irq_mask1 = TPS_REG_INT_DATA_STATUS_UPDATE |
+		     TPS_REG_INT_PLUG_EVENT,
+	.tps_struct_size = sizeof(struct tps6598x),
+	.register_port = tps6598x_register_port,
+	.unregister_port = tps6598x_unregister_port,
+	.trace_data_status = trace_tps6598x_data_status,
+	.trace_power_status = trace_tps6598x_power_status,
+	.trace_status = trace_tps6598x_status,
+	.apply_patch = tps6598x_apply_patch,
+	.init = tps6598x_init,
+	.read_data_status = tps6598x_read_data_status,
+	.read_power_status = tps66993_read_power_status,
+	.reset = tps6598x_reset,
+	.connect = tps6598x_connect,
+};
+
 static const struct tipd_data tps25750_data = {
 	.irq_handler = tps25750_interrupt,
 	.irq_mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
@@ -2028,6 +2087,7 @@ static const struct tipd_data tps25750_data = {
 
 static const struct of_device_id tps6598x_of_match[] = {
 	{ .compatible = "ti,tps6598x", &tps6598x_data},
+	{ .compatible = "ti,tps66993", &tps66993_data},
 	{ .compatible = "apple,cd321x", &cd321x_data},
 	{ .compatible = "ti,tps25750", &tps25750_data},
 	{}
-- 
2.49.1

Re: [PATCH v3 2/3] usb: typec: tipd: add read_power_status callback to tipd_data

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing cancellation of queued delayed works in the probe error paths leads to Use-After-Free of devres-managed memory.
- [High] In `tps6598x_probe()`, a failure in `tps->data->connect()` causes probe to fail in polling mode but silently succeed in IRQ mode.
- [High] Multi-byte raw I2C data is read into native integers (e.g., u32, u64) without endianness conversions, breaking bitwise operations on big-endian architectures.
--

--- Patch [2]: [PATCH v3 2/3] usb: typec: tipd: add read_power_status callback to tipd_data ---
commit 5bbdcd3daa173c06fcfc36de1b9d3a33bfefea78
Author: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Subject: usb: typec: tipd: add read_power_status callback to tipd_data

This commit converts direct tps6598x_read_power_status() calls to use an
indirect callback through tipd_data. This allows variants to provide their
own power status reading logic while keeping existing behavior unchanged.

> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index b6335b36d3846..9097b0d40a71d 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c

[ ... ]

> @@ -1026,7 +1027,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  		goto err_unlock;
>  
>  	if ((event1[0] | event2[0]) & TPS_REG_INT_POWER_STATUS_UPDATE)
> -		if (!tps6598x_read_power_status(tps))
> +		if (!tps->data->read_power_status(tps))
>  			goto err_unlock;
>  
>  	if ((event1[0] | event2[0]) & TPS_REG_INT_DATA_STATUS_UPDATE)

[Severity: High]
This is a pre-existing issue, but does evaluating the unswapped 64-bit I2C
data cause bitwise operations to check the wrong bits on big-endian CPUs?

When multi-byte registers are read in tps6598x_block_read(), raw I2C bytes
are copied directly into native integers using memcpy(). Because bitmasks
like TPS_REG_INT_POWER_STATUS_UPDATE are applied to event1[0] as a native
integer, the driver may evaluate the wrong bytes and malfunction on
big-endian architectures.

[ ... ]

> @@ -1839,7 +1840,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  
>  	if (status & TPS_STATUS_PLUG_PRESENT) {
>  		ret = -EINVAL;
> -		if (!tps6598x_read_power_status(tps))
> +		if (!tps->data->read_power_status(tps))
>  			goto err_unregister_port;
>  		if (!tps->data->read_data_status(tps))
>  			goto err_unregister_port;

[Severity: High]
This is a pre-existing issue, but does probe silently succeed if partner
connection fails in IRQ mode?

Slightly later in tps6598x_probe():

    ret = tps->data->connect(tps, status);
    if (ret)
        dev_err(&client->dev, "failed to register partner\n");
    }

    if (client->irq) {
        ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, ...

If tps->data->connect() returns an error, the ret value is unconditionally
overwritten when client->irq is present. This completely masks the
connection error, leaving the device registered in a degraded state without
a partner. If polling mode is used instead, the same error correctly causes
probe to abort entirely.

[Severity: Critical]
This is another pre-existing issue in tps6598x_probe(), but can the probe
error handling path cause a Use-After-Free?

If probe jumps to err_disconnect after delayed works have been queued
(e.g., tps->wq_poll or cd321x->update_work), the error path executes:

err_disconnect:
    tps6598x_disconnect(tps, 0);
err_unregister_port:
    tps->data->unregister_port(tps);
...

This cleanup completely omits canceling the pending works. The devres
cleanup will subsequently free the tps structure, but the delayed works
remain active in the system workqueue. When they later execute, they will
access the freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714061820.537792-1-radhey.shyam.pandey@amd.com?part=2

Re: [PATCH v3 3/3] usb: typec: tipd: add TPS66993 support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Redundant I2C read of the power status register for legacy TPS6598x controllers on plug events.

Pre-existing issues:
- [High] Background work items (`wq_poll` and `cd321x->update_work`) are queued during `tps6598x_probe()` but are not cancelled if the probe function subsequently fails, leading to Use-After-Free.
--

--- Patch [3]: [PATCH v3 3/3] usb: typec: tipd: add TPS66993 support ---
commit 4f820239b3f04ea4ac3411ddf0ac69f6736b3b7b
Author: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>

usb: typec: tipd: add TPS66993 support

Derive power status from the STATUS register (0x1A) now that TPS66993
deprecates the Power_Status register (0x3F). Add support for the "APP1"
mode string. TPS66993 controller is configured in polling mode and only
type-c flip orientation feature is supported on AMD Versal AI Edge Gen 2
VEK385 Evaluation Kit.

> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 9097b0d40a71d..522f56742aa9b 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
[ ... ]
> @@ -1026,6 +1056,8 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  	if (!tps6598x_read_status(tps, &status))
>  		goto err_unlock;
>  
> +	tps->status = status;
> +
>  	if ((event1[0] | event2[0]) & TPS_REG_INT_POWER_STATUS_UPDATE)
>  		if (!tps->data->read_power_status(tps))
>  			goto err_unlock;
> @@ -1034,9 +1066,15 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  		if (!tps->data->read_data_status(tps))
>  			goto err_unlock;
>  
> -	/* Handle plug insert or removal */
> -	if ((event1[0] | event2[0]) & TPS_REG_INT_PLUG_EVENT)
> +	/*
> +	 * Refresh power status before connect - needed for TPS66993 which
> +	 * synthesizes pwr_status from STATUS and never gets POWER_STATUS_UPDATE.
> +	 */
> +	if ((event1[0] | event2[0]) & TPS_REG_INT_PLUG_EVENT) {
> +		if (!tps->data->read_power_status(tps))
> +			goto err_unlock;

[Severity: Low]
Does this cause a redundant I2C read for legacy controllers?

For legacy TPS6598x controllers, a plug event typically sets both
TPS_REG_INT_POWER_STATUS_UPDATE and TPS_REG_INT_PLUG_EVENT concurrently.

Since tps->data->read_power_status(tps) is already called earlier in this
function when TPS_REG_INT_POWER_STATUS_UPDATE is set, calling it again here
without checking if it was already read results in an unnecessary I2C block
read during physical plug events.

>  		tps6598x_handle_plug_event(tps, status);
> +	}
>  
>  err_unlock:
>  	mutex_unlock(&tps->lock);
[ ... ]
> @@ -1813,6 +1852,8 @@ static int tps6598x_probe(struct i2c_client *client)
>  		goto err_clear_mask;
>  	}
>  
> +	tps->status = status;
> +
>  	/*
>  	 * This fwnode has a "compatible" property, but is never populated as a
>  	 * struct device. Instead we simply parse it to read the properties.

[Severity: High]
This is a pre-existing issue, but there appears to be a use-after-free in the
probe error path when handling delayed work items.

If the device uses polling mode (client->irq == 0), the probe function queues
wq_poll:

drivers/usb/typec/tipd/core.c:tps6598x_probe() {
    ...
    if (client->irq) {
        ...
    } else {
        dev_dbg(tps->dev, "no IRQ specified, using polling mode\n");
        INIT_DELAYED_WORK(&tps->wq_poll, tps6598x_poll_work);
        queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
                           msecs_to_jiffies(POLL_INTERVAL));
    }

    if (ret)
        goto err_disconnect;
    ...
}

If connect() failed earlier, ret holds a negative error code, and the probe
jumps to the error path:

drivers/usb/typec/tipd/core.c:tps6598x_probe() {
    ...
err_disconnect:
    tps6598x_disconnect(tps, 0);
err_unregister_port:
    tps->data->unregister_port(tps);
    ...
}

The cleanup path fails to cancel the pending delayed work. The tps structure
is then freed by devres when the probe returns the error, causing the delayed
work to dereference freed memory when it executes.

Does this same missing cleanup apply to cd321x->update_work if connect()
succeeds but devm_request_threaded_irq() subsequently fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714061820.537792-1-radhey.shyam.pandey@amd.com?part=3