[PATCH v5 00/15] Add TPS25750 USB type-C PD controller support

[PATCH v5 00/15] Add TPS25750 USB type-C PD controller support

[Abdel Alkuor]

From: Abdel Alkuor <abdelalkuor@geotab.com>

TPS25750 USB type-C PD controller has the same register offsets as
tps6598x. The following is a summary of incorporating TPS25750 into
TPS6598x driver:

- Only Check VID register (0x00) for TPS6598x and cd321x, as TPS25750 doesn't
  have VID register.

- TypeC port registration will be registered differently for each PD
  controller. TPS6598x uses system configuration register (0x28) to get
  pr/dr capabilities. On the other hand, TPS25750 will use data role property
  and PD status register (0x40) to get pr/dr capabilities as TPS25750 doesn't
  have register 0x28 supported.

- TPS25750 requires writing a binary configuration to switch PD
  controller from PTCH mode to APP mode which needs the following changes:
  - Add PTCH mode to the modes list.
  - Add an argument to tps6598x_check_mode to return the current mode.
  - Currently, tps6598x_exec_cmd has cmd timeout hardcoded to 1 second,
    and doesn't wait before checking DATA_OUT response. In TPS25750, patch 4CCs
    take longer than 1 second to execute and some requires a delay before
    checking DATA_OUT. To accommodate that, cmd_timeout and response_delay will
    be added as arguments to tps6598x_exec_cmd.
  - Implement applying patch sequence for TPS25750.

- In pm suspend callback, patch mode needs to be checked and the binary
  configuration should be applied if needed.

- For interrupt, TPS25750 has only one event register (0x14) and one mask
  register (0x16) of 11 bytes each, where TPS6598x has two event
  and two mask registers of 8 bytes each. Both TPS25750 and TPS65986x
  shares the same bit field offsets for events/masks/clear but many of
  there fields are reserved in TPS25750, the following needs to be done in
  tps6598x_interrupt:
  - Read EVENT1 register as a block of 11 bytes when tps25750 is present
  - Write CLEAR1 register as a block of 11 bytes when tps25750 is present
  - Add trace_tps25750_irq
  - During testing, I noticed that when a cable is plugged into the PD
    controller and before PD controller switches to APP mode, there is a
    lag between dr/pr updates and PlugInsertOrRemoval Event, so a check
    for dr/pr change needs to be added along TPS_REG_INT_PLUG_EVENT check

- Add TPS25750 traces for status and power status registers. Trace for
  data register won't be added as it doesn't exist in the device.

- Configure sleep mode for TPS25750.

v5:
 - PATCH 1: Add tps25750 bindings to tps6598x
 - PATCH 2: Remove tps25750 driver and incorperate tps25750
 	    into tps6598x driver
v4:
 - PATCH 1: No change
 - PATCH 2: Fix comments style and drop of_match_ptr
v3:
 - PATCH 1: Fix node name
 - PATCH 2: Upload tps25750 driver patch
v2:
 - PATCH 1: General properties clean up

Abdel Alkuor (15):
  dt-bindings: usb: tps6598x: Add tps25750
  USB: typec: Add cmd timeout and response delay
  USB: typec: Add patch mode to tps6598x
  USB: typec: Load TPS25750 patch bundle
  USB: typec: Check for EEPROM present
  USB: typec: Clear dead battery flag
  USB: typec: Apply patch again after power resume
  USB: typec: Add interrupt support for TPS25750
  USB: typec: Refactor tps6598x port registration
  USB: typec: Add port registration for tps25750
  USB: typec: Enable sleep mode for tps25750
  USB: typec: Add trace for tps25750 irq
  USB: typec: Add power status trace for tps25750
  USB: typec: Add status trace for tps25750
  USB: typec: Do not check VID for tps25750

 .../devicetree/bindings/usb/ti,tps6598x.yaml  |  70 +++
 drivers/usb/typec/tipd/core.c                 | 570 +++++++++++++++---
 drivers/usb/typec/tipd/tps6598x.h             |  36 ++
 drivers/usb/typec/tipd/trace.h                |  84 +++
 4 files changed, 683 insertions(+), 77 deletions(-)

-- 
2.34.1

[PATCH v5 01/15] dt-bindings: usb: tps6598x: Add tps25750

[Abdel Alkuor]

From: Abdel Alkuor <abdelalkuor@geotab.com>

TPS25750 is USB TypeC PD controller which is a subset of TPS6598x.

Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
---
 .../devicetree/bindings/usb/ti,tps6598x.yaml  | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
index 5497a60cddbc..e49bd92b5276 100644
--- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
@@ -20,6 +20,8 @@ properties:
     enum:
       - ti,tps6598x
       - apple,cd321x
+      - ti,tps25750
+
   reg:
     maxItems: 1
 
@@ -32,10 +34,45 @@ properties:
     items:
       - const: irq
 
+  firmware-name:
+    description: |
+      Should contain the name of the default patch binary
+      file located on the firmware search path which is
+      used to switch the controller into APP mode.
+      This is used when tps25750 doesn't have an EEPROM
+      connected to it.
+    maxItems: 1
+
+  ti,patch-address:
+    description: |
+      One of PBMs command data field is I2C slave address
+      which is used when writing the patch for TPS25750.
+      The slave address can be any value except 0x00, 0x20,
+      0x21, 0x22, and 0x23
+    $ref: /schemas/types.yaml#/definitions/uint8
+    minimum: 1
+    maximum: 0x7e
+
 required:
   - compatible
   - reg
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: ti,tps25750
+    then:
+      required:
+        - ti,patch-address
+        - connector
+
+      properties:
+        connector:
+          required:
+            - data-role
+
 additionalProperties: true
 
 examples:
@@ -68,4 +105,37 @@ examples:
             };
         };
     };
+
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        typec@21 {
+            compatible = "ti,tps25750";
+            reg = <0x21>;
+
+            interrupt-parent = <&msmgpio>;
+            interrupts = <100 IRQ_TYPE_LEVEL_LOW>;
+            interrupt-names = "irq";
+            firmware-name = "tps25750.bin";
+
+            pinctrl-names = "default";
+            pinctrl-0 = <&typec_pins>;
+
+            ti,patch-address = /bits/ 8 <0x0f>;
+
+            typec_con0: connector {
+                compatible = "usb-c-connector";
+                label = "USB-C";
+                data-role = "dual";
+                port {
+                    typec_ep0: endpoint {
+                        remote-endpoint = <&otg_ep>;
+                    };
+                };
+            };
+        };
+    };
 ...
-- 
2.34.1

[PATCH v5 02/15] USB: typec: Add cmd timeout and response delay

[Abdel Alkuor]

Some commands in tps25750 take longer than 1 second
to complete, and some responses need some delay before
the result becomes available.

Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
---
 drivers/usb/typec/tipd/core.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 37b56ce75f39..a8aee4e1aeba 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -284,7 +284,8 @@ static void tps6598x_disconnect(struct tps6598x *tps, u32 status)
 
 static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
 			     size_t in_len, u8 *in_data,
-			     size_t out_len, u8 *out_data)
+			     size_t out_len, u8 *out_data,
+			     u32 cmd_timeout_ms, u32 res_delay_ms)
 {
 	unsigned long timeout;
 	u32 val;
@@ -307,8 +308,7 @@ static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
 	if (ret < 0)
 		return ret;
 
-	/* XXX: Using 1s for now, but it may not be enough for every command. */
-	timeout = jiffies + msecs_to_jiffies(1000);
+	timeout = jiffies + msecs_to_jiffies(cmd_timeout_ms);
 
 	do {
 		ret = tps6598x_read32(tps, TPS_REG_CMD1, &val);
@@ -321,6 +321,9 @@ static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
 			return -ETIMEDOUT;
 	} while (val);
 
+	/* some commands require delay for the result to be available */
+	mdelay(res_delay_ms);
+
 	if (out_len) {
 		ret = tps6598x_block_read(tps, TPS_REG_DATA1,
 					  out_data, out_len);
@@ -354,7 +357,7 @@ static int tps6598x_dr_set(struct typec_port *port, enum typec_data_role role)
 
 	mutex_lock(&tps->lock);
 
-	ret = tps6598x_exec_cmd(tps, cmd, 0, NULL, 0, NULL);
+	ret = tps6598x_exec_cmd(tps, cmd, 0, NULL, 0, NULL, 1000, 0);
 	if (ret)
 		goto out_unlock;
 
@@ -384,7 +387,7 @@ static int tps6598x_pr_set(struct typec_port *port, enum typec_role role)
 
 	mutex_lock(&tps->lock);
 
-	ret = tps6598x_exec_cmd(tps, cmd, 0, NULL, 0, NULL);
+	ret = tps6598x_exec_cmd(tps, cmd, 0, NULL, 0, NULL, 1000, 0);
 	if (ret)
 		goto out_unlock;
 
@@ -654,7 +657,10 @@ static int cd321x_switch_power_state(struct tps6598x *tps, u8 target_state)
 	if (state == target_state)
 		return 0;
 
-	ret = tps6598x_exec_cmd(tps, "SSPS", sizeof(u8), &target_state, 0, NULL);
+	ret = tps6598x_exec_cmd(tps, "SSPS",
+				sizeof(u8), &target_state,
+				0, NULL,
+				1000, 0);
 	if (ret)
 		return ret;
 
-- 
2.34.1

[PATCH v5 03/15] USB: typec: Add patch mode to tps6598x

[Abdel Alkuor]

TPS25750 has a patch mode indicating the device requires
a configuration to get the device into operational mode

Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
---
 drivers/usb/typec/tipd/core.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index a8aee4e1aeba..6d2151325fbb 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -68,6 +68,7 @@ enum {
 	TPS_MODE_BOOT,
 	TPS_MODE_BIST,
 	TPS_MODE_DISC,
+	TPS_MODE_PTCH,
 };
 
 static const char *const modes[] = {
@@ -75,6 +76,7 @@ static const char *const modes[] = {
 	[TPS_MODE_BOOT]	= "BOOT",
 	[TPS_MODE_BIST]	= "BIST",
 	[TPS_MODE_DISC]	= "DISC",
+	[TPS_MODE_PTCH] = "PTCH",
 };
 
 /* Unrecognized commands will be replaced with "!CMD" */
@@ -576,7 +578,7 @@ static void tps6598x_poll_work(struct work_struct *work)
 			   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
 }
 
-static int tps6598x_check_mode(struct tps6598x *tps)
+static int tps6598x_check_mode(struct tps6598x *tps, u8 *curr_mode)
 {
 	char mode[5] = { };
 	int ret;
@@ -585,8 +587,11 @@ static int tps6598x_check_mode(struct tps6598x *tps)
 	if (ret)
 		return ret;
 
-	switch (match_string(modes, ARRAY_SIZE(modes), mode)) {
+	*curr_mode = match_string(modes, ARRAY_SIZE(modes), mode);
+
+	switch (*curr_mode) {
 	case TPS_MODE_APP:
+	case TPS_MODE_PTCH:
 		return 0;
 	case TPS_MODE_BOOT:
 		dev_warn(tps->dev, "dead-battery condition\n");
@@ -715,6 +720,7 @@ static int tps6598x_probe(struct i2c_client *client)
 	u32 vid;
 	int ret;
 	u64 mask1;
+	u8 mode;
 
 	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
 	if (!tps)
@@ -759,7 +765,7 @@ static int tps6598x_probe(struct i2c_client *client)
 
 	tps->irq_handler = irq_handler;
 	/* Make sure the controller has application firmware running */
-	ret = tps6598x_check_mode(tps);
+	ret = tps6598x_check_mode(tps, &mode);
 	if (ret)
 		return ret;
 
-- 
2.34.1

[PATCH v5 04/15] USB: typec: Load TPS25750 patch bundle

[Abdel Alkuor]

From: Abdel Alkuor <abdelalkuor@geotab.com>

TPS25750 controller requires a binary to be loaded with a configuration
binary by an EEPROM or a host.

Appling a patch bundling using a host is implemented based on the flow
diagram pg.62 in TPS25750 host interface manual.
https://www.ti.com/lit/ug/slvuc05a/slvuc05a.pdf

The flow diagram can be summarized as following:
- Start the patch loading sequence with patch bundle information by
  executing PBMs
- Write the whole patch at once
- When writing the patch fails, execute PBMe which instructs the PD controller
  to end the patching process
- After writing the patch successfully, execute PBMc which verifies the patch
  integrity and applies the patch internally
- Wait for the device to switch into APP mode (normal operation)

The execuation flow diagram polls the events register and then polls the
corresponding register related to the event as well before advancing to the next
state. Polling the events register is a redundant step, in this implementation
only the corresponding register related to the event is polled.

Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
---
 drivers/usb/typec/tipd/core.c | 237 +++++++++++++++++++++++++++++++++-
 1 file changed, 236 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 6d2151325fbb..fea139c72d6d 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -17,6 +17,7 @@
 #include <linux/usb/typec_altmode.h>
 #include <linux/usb/role.h>
 #include <linux/workqueue.h>
+#include <linux/firmware.h>
 
 #include "tps6598x.h"
 #include "trace.h"
@@ -43,6 +44,23 @@
 /* TPS_REG_SYSTEM_CONF bits */
 #define TPS_SYSCONF_PORTINFO(c)		((c) & 7)
 
+/*
+ * BPMs task timeout, recommended 5 seconds
+ * pg.48 TPS2575 Host Interface Technical Reference
+ * Manual (Rev. A)
+ * https://www.ti.com/lit/ug/slvuc05a/slvuc05a.pdf
+ */
+#define TPS_BUNDLE_TIMEOUT	0x32
+
+/* BPMs return code */
+#define TPS_TASK_BPMS_INVALID_BUNDLE_SIZE	0x4
+#define TPS_TASK_BPMS_INVALID_SLAVE_ADDR	0x5
+#define TPS_TASK_BPMS_INVALID_TIMEOUT		0x6
+
+/* PBMc data out */
+#define TPS_PBMC_RC	0 /* Return code */
+#define TPS_PBMC_DPCS	2 /* device patch complete status */
+
 enum {
 	TPS_PORTINFO_SINK,
 	TPS_PORTINFO_SINK_ACCESSORY,
@@ -88,6 +106,8 @@ struct tps6598x {
 	struct mutex lock; /* device lock */
 	u8 i2c_protocol:1;
 
+	u8 is_tps25750:1;
+
 	struct typec_port *port;
 	struct typec_partner *partner;
 	struct usb_pd_identity partner_identity;
@@ -708,6 +728,203 @@ static int devm_tps6598_psy_register(struct tps6598x *tps)
 	return PTR_ERR_OR_ZERO(tps->psy);
 }
 
+static int
+tps25750_write_firmware(struct tps6598x *tps,
+			u8 bpms_addr, const u8 *data, size_t len)
+{
+	struct i2c_client *client = to_i2c_client(tps->dev);
+	int ret;
+	u8 slave_addr;
+	int timeout;
+
+	slave_addr = client->addr;
+	timeout = client->adapter->timeout;
+
+	/*
+	 * binary configuration size is around ~16Kbytes
+	 * which might take some time to finish writing it
+	 */
+	client->adapter->timeout = msecs_to_jiffies(5000);
+	client->addr = bpms_addr;
+
+	ret = regmap_raw_write(tps->regmap, data[0], &data[1], len - 1);
+
+	client->addr = slave_addr;
+	client->adapter->timeout = timeout;
+
+	return ret;
+}
+
+static int
+tps25750_exec_pbms(struct tps6598x *tps, u8 *in_data, size_t in_len)
+{
+	int ret;
+	u8 rc;
+
+	ret = tps6598x_exec_cmd(tps, "PBMs", in_len, in_data,
+				sizeof(rc), &rc, 4000, 0);
+	if (ret)
+		return ret;
+
+	switch (rc) {
+	case TPS_TASK_BPMS_INVALID_BUNDLE_SIZE:
+		dev_err(tps->dev, "%s: invalid fw size\n", __func__);
+		return -EINVAL;
+	case TPS_TASK_BPMS_INVALID_SLAVE_ADDR:
+		dev_err(tps->dev, "%s: invalid slave address\n", __func__);
+		return -EINVAL;
+	case TPS_TASK_BPMS_INVALID_TIMEOUT:
+		dev_err(tps->dev, "%s: timed out\n", __func__);
+		return -ETIMEDOUT;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int tps25750_abort_patch_process(struct tps6598x *tps)
+{
+	int ret;
+	u8 mode;
+
+	ret = tps6598x_exec_cmd(tps, "PBMe", 0, NULL, 0, NULL, 1000, 0);
+	if (ret)
+		return ret;
+
+	ret = tps6598x_check_mode(tps, &mode);
+	if (mode != TPS_MODE_PTCH)
+		dev_err(tps->dev, "failed to switch to \"PTCH\" mode\n");
+
+	return ret;
+}
+
+static int tps25750_start_patch_burst_mode(struct tps6598x *tps)
+{
+	int ret;
+	const struct firmware *fw;
+	const char *firmware_name;
+	struct {
+		u32 fw_size;
+		u8 addr;
+		u8 timeout;
+	} __packed bpms_data;
+
+	ret = device_property_read_string(tps->dev, "firmware-name",
+					  &firmware_name);
+	if (ret)
+		return ret;
+
+	ret = request_firmware(&fw, firmware_name, tps->dev);
+	if (ret) {
+		dev_err(tps->dev, "failed to retrieve \"%s\"\n", firmware_name);
+		return ret;
+	}
+
+	if (fw->size == 0) {
+		ret = -EINVAL;
+		goto release_fw;
+	}
+
+	ret = device_property_read_u8(tps->dev, "ti,patch-address", &bpms_data.addr);
+	if (ret) {
+		dev_err(tps->dev, "failed to get patch address\n");
+		return ret;
+	}
+
+	bpms_data.fw_size = fw->size;
+	bpms_data.timeout = TPS_BUNDLE_TIMEOUT;
+
+	ret = tps25750_exec_pbms(tps, (u8 *)&bpms_data, sizeof(bpms_data));
+	if (ret)
+		goto release_fw;
+
+	ret = tps25750_write_firmware(tps, bpms_data.addr, fw->data, fw->size);
+	if (ret) {
+		dev_err(tps->dev, "Failed to write patch %s of %lu bytes\n",
+			firmware_name, fw->size);
+		goto release_fw;
+	}
+
+	/*
+	 * A delay of 500us is required after the firmware is written
+	 * based on pg.62 in tps6598x Host Interface Technical
+	 * Reference Manual
+	 * https://www.ti.com/lit/ug/slvuc05a/slvuc05a.pdf
+	 */
+	udelay(500);
+
+release_fw:
+	release_firmware(fw);
+
+	return ret;
+}
+
+static int tps25750_complete_patch_process(struct tps6598x *tps)
+{
+	int ret;
+	u8 out_data[40];
+	u8 dummy[2] = { };
+
+	/*
+	 * Without writing something to DATA_IN, this command would
+	 * return an error
+	 */
+	ret = tps6598x_exec_cmd(tps, "PBMc", sizeof(dummy), dummy,
+				sizeof(out_data), out_data, 2000, 20);
+	if (ret)
+		return ret;
+
+	if (out_data[TPS_PBMC_RC]) {
+		dev_err(tps->dev,
+			"%s: pbmc failed: %u\n", __func__,
+			out_data[TPS_PBMC_RC]);
+		return -EIO;
+	}
+
+	if (out_data[TPS_PBMC_DPCS]) {
+		dev_err(tps->dev,
+			"%s: failed device patch complete status: %u\n",
+			__func__, out_data[TPS_PBMC_DPCS]);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int tps25750_apply_patch(struct tps6598x *tps)
+{
+	int ret;
+	unsigned long timeout;
+	u8 mode;
+
+	ret = tps25750_start_patch_burst_mode(tps);
+	if (ret) {
+		tps25750_abort_patch_process(tps);
+		return ret;
+	}
+
+	ret = tps25750_complete_patch_process(tps);
+	if (ret)
+		return ret;
+
+	timeout = jiffies + msecs_to_jiffies(1000);
+
+	do {
+		ret = tps6598x_check_mode(tps, &mode);
+		if (ret)
+			return ret;
+
+		if (time_is_before_jiffies(timeout))
+			return -ETIMEDOUT;
+
+	} while (mode != TPS_MODE_APP);
+
+	dev_info(tps->dev, "controller switched to \"APP\" mode\n");
+
+	return 0;
+};
+
 static int tps6598x_probe(struct i2c_client *client)
 {
 	irq_handler_t irq_handler = tps6598x_interrupt;
@@ -757,6 +974,8 @@ static int tps6598x_probe(struct i2c_client *client)
 
 		irq_handler = cd321x_interrupt;
 	} else {
+
+		tps->is_tps25750 = of_device_is_compatible(np, "ti,tps25750");
 		/* Enable power status, data status and plug event interrupts */
 		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
 			TPS_REG_INT_DATA_STATUS_UPDATE |
@@ -769,9 +988,15 @@ static int tps6598x_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	if (tps->is_tps25750 && mode == TPS_MODE_PTCH) {
+		ret = tps25750_apply_patch(tps);
+		if (ret)
+			return ret;
+	}
+
 	ret = tps6598x_write64(tps, TPS_REG_INT_MASK1, mask1);
 	if (ret)
-		return ret;
+		goto err_reset_controller;
 
 	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
 	if (ret < 0)
@@ -891,6 +1116,10 @@ static int tps6598x_probe(struct i2c_client *client)
 	fwnode_handle_put(fwnode);
 err_clear_mask:
 	tps6598x_write64(tps, TPS_REG_INT_MASK1, 0);
+err_reset_controller:
+	/* Reset PD controller to remove any applied patch */
+	if (tps->is_tps25750)
+		tps6598x_exec_cmd(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
 	return ret;
 }
 
@@ -901,9 +1130,14 @@ static void tps6598x_remove(struct i2c_client *client)
 	if (!client->irq)
 		cancel_delayed_work_sync(&tps->wq_poll);
 
+	devm_free_irq(tps->dev, client->irq, tps);
 	tps6598x_disconnect(tps, 0);
 	typec_unregister_port(tps->port);
 	usb_role_switch_put(tps->role_sw);
+
+	/* Reset PD controller to remove any applied patch */
+	if (tps->is_tps25750)
+		tps6598x_exec_cmd(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
 }
 
 static int __maybe_unused tps6598x_suspend(struct device *dev)
@@ -946,6 +1180,7 @@ static const struct dev_pm_ops tps6598x_pm_ops = {
 static const struct of_device_id tps6598x_of_match[] = {
 	{ .compatible = "ti,tps6598x", },
 	{ .compatible = "apple,cd321x", },
+	{ .compatible = "ti,tps25750", },
 	{}
 };
 MODULE_DEVICE_TABLE(of, tps6598x_of_match);
-- 
2.34.1

[PATCH v5 05/15] USB: typec: Check for EEPROM present

[Abdel Alkuor]

From: Abdel Alkuor <abdelalkuor@geotab.com>

When an EEPROM is present, tps25750 loads the binary configuration from
EEPROM. Hence, all we need to do is wait for the device to switch to APP
mode

Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
---
 drivers/usb/typec/tipd/core.c     | 13 +++++++++++++
 drivers/usb/typec/tipd/tps6598x.h |  3 +++
 2 files changed, 16 insertions(+)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index fea139c72d6d..b3d4b2b5bf5f 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -37,6 +37,7 @@
 #define TPS_REG_STATUS			0x1a
 #define TPS_REG_SYSTEM_CONF		0x28
 #define TPS_REG_CTRL_CONF		0x29
+#define TPS_REG_BOOT_STATUS		0x2D
 #define TPS_REG_POWER_STATUS		0x3f
 #define TPS_REG_RX_IDENTITY_SOP		0x48
 #define TPS_REG_DATA_STATUS		0x5f
@@ -897,6 +898,17 @@ static int tps25750_apply_patch(struct tps6598x *tps)
 	int ret;
 	unsigned long timeout;
 	u8 mode;
+	u64 status = 0;
+
+	ret = tps6598x_block_read(tps, TPS_REG_BOOT_STATUS, &status, 5);
+	if (ret)
+		return ret;
+	/*
+	 * Nothing to be done if the configuration
+	 * is being loaded from EERPOM
+	 */
+	if (status & TPS25750_BOOT_STATUS_I2C_EEPROM_PRESENT)
+		goto wait_for_app;
 
 	ret = tps25750_start_patch_burst_mode(tps);
 	if (ret) {
@@ -908,6 +920,7 @@ static int tps25750_apply_patch(struct tps6598x *tps)
 	if (ret)
 		return ret;
 
+wait_for_app:
 	timeout = jiffies + msecs_to_jiffies(1000);
 
 	do {
diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
index 527857549d69..5e942c089c27 100644
--- a/drivers/usb/typec/tipd/tps6598x.h
+++ b/drivers/usb/typec/tipd/tps6598x.h
@@ -199,4 +199,7 @@
 #define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_A    BIT(2)
 #define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_B    (BIT(2) | BIT(1))
 
+/* BOOT STATUS REG*/
+#define TPS25750_BOOT_STATUS_I2C_EEPROM_PRESENT	BIT(3)
+
 #endif /* __TPS6598X_H__ */
-- 
2.34.1

[PATCH v5 06/15] USB: typec: Clear dead battery flag

[Abdel Alkuor]

From: Abdel Alkuor <abdelalkuor@geotab.com>

Dead battery flag must be cleared after switching tps25750 to APP mode
so the PD controller becomes fully functional.

Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
---
 drivers/usb/typec/tipd/core.c     | 16 ++++++++++++++++
 drivers/usb/typec/tipd/tps6598x.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index b3d4b2b5bf5f..3ad8112c78b6 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -933,6 +933,22 @@ static int tps25750_apply_patch(struct tps6598x *tps)
 
 	} while (mode != TPS_MODE_APP);
 
+	/*
+	 * The dead battery flag may be triggered when the controller
+	 * port is connected to a device that can source power and
+	 * attempts to power up both the controller and the board it is on.
+	 * To restore controller functionality, it is necessary to clear
+	 * this flag
+	 */
+	if (status & TPS_BOOT_STATUS_DEAD_BATTERY_FLAG) {
+		ret = tps6598x_exec_cmd(tps, "DBfg", 0, NULL, 0, NULL, 1000, 0);
+		if (ret) {
+			dev_err(tps->dev,
+				"failed to clear dead battery %d\n", ret);
+			return ret;
+		}
+	}
+
 	dev_info(tps->dev, "controller switched to \"APP\" mode\n");
 
 	return 0;
diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
index 5e942c089c27..362e1eca53ad 100644
--- a/drivers/usb/typec/tipd/tps6598x.h
+++ b/drivers/usb/typec/tipd/tps6598x.h
@@ -200,6 +200,7 @@
 #define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_B    (BIT(2) | BIT(1))
 
 /* BOOT STATUS REG*/
+#define TPS_BOOT_STATUS_DEAD_BATTERY_FLAG	BIT(2)
 #define TPS25750_BOOT_STATUS_I2C_EEPROM_PRESENT	BIT(3)
 
 #endif /* __TPS6598X_H__ */
-- 
2.34.1

[PATCH v5 07/15] USB: typec: Apply patch again after power resume

[Abdel Alkuor]

From: Abdel Alkuor <abdelalkuor@geotab.com>

TPS25750 PD controller might be powered off externally at power suspend,
after resuming PD controller power back, apply the patch again.

Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
---
 drivers/usb/typec/tipd/core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 3ad8112c78b6..bd5436fd88fd 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -1189,6 +1189,18 @@ static int __maybe_unused tps6598x_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct tps6598x *tps = i2c_get_clientdata(client);
+	u8 mode;
+	int ret;
+
+	ret = tps6598x_check_mode(tps, &mode);
+	if (ret)
+		return ret;
+
+	if (mode == TPS_MODE_PTCH) {
+		ret = tps25750_apply_patch(tps);
+		if (ret)
+			return ret;
+	}
 
 	if (tps->wakeup) {
 		disable_irq_wake(client->irq);
-- 
2.34.1

[PATCH v5 08/15] USB: typec: Add interrupt support for TPS25750

[Abdel Alkuor]

From: Abdel Alkuor <abdelalkuor@geotab.com>

Update tps6598x interrupt handler to accommodate tps25750 interrupt

Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
---
 drivers/usb/typec/tipd/core.c | 49 +++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index bd5436fd88fd..17b3bc480f97 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -120,6 +120,7 @@ struct tps6598x {
 	enum power_supply_usb_type usb_type;
 
 	int wakeup;
+	u32 status; /* status reg */
 	u16 pwr_status;
 	struct delayed_work	wq_poll;
 	irq_handler_t irq_handler;
@@ -539,50 +540,71 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
 	return IRQ_NONE;
 }
 
+static bool tps6598x_has_role_changed(struct tps6598x *tps, u32 status)
+{
+	status ^= tps->status;
+
+	return status & (TPS_STATUS_PORTROLE | TPS_STATUS_DATAROLE);
+}
+
 static irqreturn_t tps6598x_interrupt(int irq, void *data)
 {
 	struct tps6598x *tps = data;
-	u64 event1 = 0;
-	u64 event2 = 0;
+	u64 event[2] = { };
 	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);
+	if (tps->is_tps25750) {
+		ret = tps6598x_block_read(tps, TPS_REG_INT_EVENT1, event, 11);
+	} else {
+		ret = tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event[0]);
+		ret |= tps6598x_read64(tps, TPS_REG_INT_EVENT2, &event[1]);
+	}
+
 	if (ret) {
 		dev_err(tps->dev, "%s: failed to read events\n", __func__);
 		goto err_unlock;
 	}
-	trace_tps6598x_irq(event1, event2);
+	trace_tps6598x_irq(event[0], event[1]);
 
-	if (!(event1 | event2))
+	if (!(event[0] | event[1]))
 		goto err_unlock;
 
 	if (!tps6598x_read_status(tps, &status))
 		goto err_clear_ints;
 
-	if ((event1 | event2) & TPS_REG_INT_POWER_STATUS_UPDATE)
+	if ((event[0] | event[1]) & 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 ((event[0] | event[1]) & 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)
+	/*
+	 * data/port roles could be updated independently after
+	 * 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 ||
+		tps6598x_has_role_changed(tps, status))
 		tps6598x_handle_plug_event(tps, status);
 
+	tps->status = status;
 err_clear_ints:
-	tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event1);
-	tps6598x_write64(tps, TPS_REG_INT_CLEAR2, event2);
+	if (tps->is_tps25750) {
+		tps6598x_block_write(tps, TPS_REG_INT_CLEAR1, event, 11);
+	} else {
+		tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event[0]);
+		tps6598x_write64(tps, TPS_REG_INT_CLEAR2, event[1]);
+	}
 
 err_unlock:
 	mutex_unlock(&tps->lock);
 
-	if (event1 | event2)
+	if (event[0] | event[1])
 		return IRQ_HANDLED;
 	return IRQ_NONE;
 }
@@ -1003,7 +1025,6 @@ static int tps6598x_probe(struct i2c_client *client)
 
 		irq_handler = cd321x_interrupt;
 	} else {
-
 		tps->is_tps25750 = of_device_is_compatible(np, "ti,tps25750");
 		/* Enable power status, data status and plug event interrupts */
 		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
-- 
2.34.1

[PATCH v5 09/15] USB: typec: Refactor tps6598x port registration

[Abdel Alkuor]

From: Abdel Alkuor <abdelalkuor@geotab.com>

tps6598x and cd321x use TPS_REG_SYSTEM_CONF to get dr/pr roles
where other similar devices don't have this register such as tps25750.

Move tps6598x port registration to its own function

Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
---
 drivers/usb/typec/tipd/core.c | 99 +++++++++++++++++++----------------
 1 file changed, 54 insertions(+), 45 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 17b3bc480f97..8218d88a4a06 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -976,15 +976,65 @@ static int tps25750_apply_patch(struct tps6598x *tps)
 	return 0;
 };
 
+static int
+tps6598x_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
+{
+	int ret;
+	u32 conf;
+	struct typec_capability typec_cap = { };
+
+	ret = tps6598x_read32(tps, TPS_REG_SYSTEM_CONF, &conf);
+	if (ret)
+		return ret;
+
+	typec_cap.revision = USB_TYPEC_REV_1_2;
+	typec_cap.pd_revision = 0x200;
+	typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
+	typec_cap.driver_data = tps;
+	typec_cap.ops = &tps6598x_ops;
+	typec_cap.fwnode = fwnode;
+
+	switch (TPS_SYSCONF_PORTINFO(conf)) {
+	case TPS_PORTINFO_SINK_ACCESSORY:
+	case TPS_PORTINFO_SINK:
+		typec_cap.type = TYPEC_PORT_SNK;
+		typec_cap.data = TYPEC_PORT_UFP;
+		break;
+	case TPS_PORTINFO_DRP_UFP_DRD:
+	case TPS_PORTINFO_DRP_DFP_DRD:
+		typec_cap.type = TYPEC_PORT_DRP;
+		typec_cap.data = TYPEC_PORT_DRD;
+		break;
+	case TPS_PORTINFO_DRP_UFP:
+		typec_cap.type = TYPEC_PORT_DRP;
+		typec_cap.data = TYPEC_PORT_UFP;
+		break;
+	case TPS_PORTINFO_DRP_DFP:
+		typec_cap.type = TYPEC_PORT_DRP;
+		typec_cap.data = TYPEC_PORT_DFP;
+		break;
+	case TPS_PORTINFO_SOURCE:
+		typec_cap.type = TYPEC_PORT_SRC;
+		typec_cap.data = TYPEC_PORT_DFP;
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	tps->port = typec_register_port(tps->dev, &typec_cap);
+	if (IS_ERR(tps->port))
+		return PTR_ERR(tps->port);
+
+	return 0;
+}
+
 static int tps6598x_probe(struct i2c_client *client)
 {
 	irq_handler_t irq_handler = tps6598x_interrupt;
 	struct device_node *np = client->dev.of_node;
-	struct typec_capability typec_cap = { };
 	struct tps6598x *tps;
 	struct fwnode_handle *fwnode;
 	u32 status;
-	u32 conf;
 	u32 vid;
 	int ret;
 	u64 mask1;
@@ -1053,10 +1103,6 @@ static int tps6598x_probe(struct i2c_client *client)
 		goto err_clear_mask;
 	trace_tps6598x_status(status);
 
-	ret = tps6598x_read32(tps, TPS_REG_SYSTEM_CONF, &conf);
-	if (ret < 0)
-		goto err_clear_mask;
-
 	/*
 	 * This fwnode has a "compatible" property, but is never populated as a
 	 * struct device. Instead we simply parse it to read the properties.
@@ -1074,50 +1120,13 @@ static int tps6598x_probe(struct i2c_client *client)
 		goto err_fwnode_put;
 	}
 
-	typec_cap.revision = USB_TYPEC_REV_1_2;
-	typec_cap.pd_revision = 0x200;
-	typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
-	typec_cap.driver_data = tps;
-	typec_cap.ops = &tps6598x_ops;
-	typec_cap.fwnode = fwnode;
-
-	switch (TPS_SYSCONF_PORTINFO(conf)) {
-	case TPS_PORTINFO_SINK_ACCESSORY:
-	case TPS_PORTINFO_SINK:
-		typec_cap.type = TYPEC_PORT_SNK;
-		typec_cap.data = TYPEC_PORT_UFP;
-		break;
-	case TPS_PORTINFO_DRP_UFP_DRD:
-	case TPS_PORTINFO_DRP_DFP_DRD:
-		typec_cap.type = TYPEC_PORT_DRP;
-		typec_cap.data = TYPEC_PORT_DRD;
-		break;
-	case TPS_PORTINFO_DRP_UFP:
-		typec_cap.type = TYPEC_PORT_DRP;
-		typec_cap.data = TYPEC_PORT_UFP;
-		break;
-	case TPS_PORTINFO_DRP_DFP:
-		typec_cap.type = TYPEC_PORT_DRP;
-		typec_cap.data = TYPEC_PORT_DFP;
-		break;
-	case TPS_PORTINFO_SOURCE:
-		typec_cap.type = TYPEC_PORT_SRC;
-		typec_cap.data = TYPEC_PORT_DFP;
-		break;
-	default:
-		ret = -ENODEV;
-		goto err_role_put;
-	}
-
 	ret = devm_tps6598_psy_register(tps);
 	if (ret)
 		goto err_role_put;
 
-	tps->port = typec_register_port(&client->dev, &typec_cap);
-	if (IS_ERR(tps->port)) {
-		ret = PTR_ERR(tps->port);
+	ret = tps6598x_register_port(tps, fwnode);
+	if (ret)
 		goto err_role_put;
-	}
 
 	if (status & TPS_STATUS_PLUG_PRESENT) {
 		ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &tps->pwr_status);
-- 
2.34.1

[PATCH v5 10/15] USB: typec: Add port registration for tps25750

[Abdel Alkuor]

From: Abdel Alkuor <abdelalkuor@geotab.com>

TPS25750 doesn't have system configuration register to get dr/pr of the
current applied binary configuration.

Get data role from the device node and power role from PD status register.

Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
---
 drivers/usb/typec/tipd/core.c     | 61 ++++++++++++++++++++++++++++++-
 drivers/usb/typec/tipd/tps6598x.h | 10 +++++
 2 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 8218d88a4a06..a97fda68cb54 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -39,6 +39,7 @@
 #define TPS_REG_CTRL_CONF		0x29
 #define TPS_REG_BOOT_STATUS		0x2D
 #define TPS_REG_POWER_STATUS		0x3f
+#define TPS_REG_PD_STATUS		0x40
 #define TPS_REG_RX_IDENTITY_SOP		0x48
 #define TPS_REG_DATA_STATUS		0x5f
 
@@ -1028,6 +1029,60 @@ tps6598x_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
 	return 0;
 }
 
+static int
+tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
+{
+	struct typec_capability typec_cap = { };
+	const char *data_role;
+	u8 pd_status;
+	int ret;
+
+	ret = tps6598x_read8(tps, TPS_REG_PD_STATUS, &pd_status);
+	if (ret)
+		return ret;
+
+	ret = fwnode_property_read_string(fwnode, "data-role", &data_role);
+	if (ret) {
+		dev_err(tps->dev, "data-role not found: %d\n", ret);
+		return ret;
+	}
+
+	ret = typec_find_port_data_role(data_role);
+	if (ret < 0) {
+		dev_err(tps->dev, "unknown data-role: %s\n", data_role);
+		return ret;
+	}
+
+	typec_cap.data = ret;
+	typec_cap.revision = USB_TYPEC_REV_1_3;
+	typec_cap.pd_revision = 0x300;
+	typec_cap.driver_data = tps;
+	typec_cap.ops = &tps6598x_ops;
+	typec_cap.fwnode = fwnode;
+	typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
+
+	switch (TPS_PD_STATUS_PORT_TYPE(pd_status)) {
+	case TPS_PD_STATUS_PORT_TYPE_SINK_SOURCE:
+	case TPS_PD_STATUS_PORT_TYPE_SOURCE_SINK:
+		typec_cap.type = TYPEC_PORT_DRP;
+		break;
+	case TPS_PD_STATUS_PORT_TYPE_SINK:
+		typec_cap.type = TYPEC_PORT_SNK;
+		break;
+	case TPS_PD_STATUS_PORT_TYPE_SOURCE:
+		typec_cap.type = TYPEC_PORT_SRC;
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	tps->port = typec_register_port(tps->dev, &typec_cap);
+	if (IS_ERR(tps->port))
+		return PTR_ERR(tps->port);
+
+	return 0;
+}
+
 static int tps6598x_probe(struct i2c_client *client)
 {
 	irq_handler_t irq_handler = tps6598x_interrupt;
@@ -1124,7 +1179,11 @@ static int tps6598x_probe(struct i2c_client *client)
 	if (ret)
 		goto err_role_put;
 
-	ret = tps6598x_register_port(tps, fwnode);
+	if (np && of_device_is_compatible(np, "ti,tps25750"))
+		ret = tps25750_register_port(tps, fwnode);
+	else
+		ret = tps6598x_register_port(tps, fwnode);
+
 	if (ret)
 		goto err_role_put;
 
diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
index 362e1eca53ad..d2c65387994f 100644
--- a/drivers/usb/typec/tipd/tps6598x.h
+++ b/drivers/usb/typec/tipd/tps6598x.h
@@ -203,4 +203,14 @@
 #define TPS_BOOT_STATUS_DEAD_BATTERY_FLAG	BIT(2)
 #define TPS25750_BOOT_STATUS_I2C_EEPROM_PRESENT	BIT(3)
 
+/* PD STATUS REG */
+#define TPS_REG_PD_STATUS_PORT_TYPE_MASK	GENMASK(5, 4)
+#define TPS_PD_STATUS_PORT_TYPE(x) \
+	TPS_FIELD_GET(TPS_REG_PD_STATUS_PORT_TYPE_MASK, x)
+
+#define TPS_PD_STATUS_PORT_TYPE_SINK_SOURCE	0
+#define TPS_PD_STATUS_PORT_TYPE_SINK		1
+#define TPS_PD_STATUS_PORT_TYPE_SOURCE		2
+#define TPS_PD_STATUS_PORT_TYPE_SOURCE_SINK	3
+
 #endif /* __TPS6598X_H__ */
-- 
2.34.1

[PATCH v5 11/15] USB: typec: Enable sleep mode for tps25750

[Abdel Alkuor]

From: Abdel Alkuor <abdelalkuor@geotab.com>

Allow controller to enter sleep mode after the device
is idle for sleep time.

Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
---
 drivers/usb/typec/tipd/core.c     | 29 ++++++++++++++++++++++++++++-
 drivers/usb/typec/tipd/tps6598x.h |  3 +++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index a97fda68cb54..3d9877551160 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -42,6 +42,7 @@
 #define TPS_REG_PD_STATUS		0x40
 #define TPS_REG_RX_IDENTITY_SOP		0x48
 #define TPS_REG_DATA_STATUS		0x5f
+#define TPS_REG_SLEEP_CONF		0x70
 
 /* TPS_REG_SYSTEM_CONF bits */
 #define TPS_SYSCONF_PORTINFO(c)		((c) & 7)
@@ -205,6 +206,11 @@ static inline int tps6598x_read64(struct tps6598x *tps, u8 reg, u64 *val)
 	return tps6598x_block_read(tps, reg, val, sizeof(u64));
 }
 
+static inline int tps6598x_write8(struct tps6598x *tps, u8 reg, u8 val)
+{
+	return tps6598x_block_write(tps, reg, &val, sizeof(u8));
+}
+
 static inline int tps6598x_write64(struct tps6598x *tps, u8 reg, u64 val)
 {
 	return tps6598x_block_write(tps, reg, &val, sizeof(u64));
@@ -977,6 +983,24 @@ static int tps25750_apply_patch(struct tps6598x *tps)
 	return 0;
 };
 
+static int tps25750_init(struct tps6598x *tps)
+{
+	int ret;
+
+	ret = tps25750_apply_patch(tps);
+	if (ret)
+		return ret;
+
+	ret = tps6598x_write8(tps, TPS_REG_SLEEP_CONF,
+			      TPS_SLEEP_CONF_SLEEP_MODE_ALLOWED);
+	if (ret)
+		dev_warn(tps->dev,
+			 "%s: failed to enable sleep mode: %d\n",
+			 __func__, ret);
+
+	return 0;
+}
+
 static int
 tps6598x_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
 {
@@ -1131,6 +1155,7 @@ static int tps6598x_probe(struct i2c_client *client)
 		irq_handler = cd321x_interrupt;
 	} else {
 		tps->is_tps25750 = of_device_is_compatible(np, "ti,tps25750");
+
 		/* Enable power status, data status and plug event interrupts */
 		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
 			TPS_REG_INT_DATA_STATUS_UPDATE |
@@ -1138,6 +1163,7 @@ static int tps6598x_probe(struct i2c_client *client)
 	}
 
 	tps->irq_handler = irq_handler;
+
 	/* Make sure the controller has application firmware running */
 	ret = tps6598x_check_mode(tps, &mode);
 	if (ret)
@@ -1149,6 +1175,7 @@ static int tps6598x_probe(struct i2c_client *client)
 			return ret;
 	}
 
+
 	ret = tps6598x_write64(tps, TPS_REG_INT_MASK1, mask1);
 	if (ret)
 		goto err_reset_controller;
@@ -1286,7 +1313,7 @@ static int __maybe_unused tps6598x_resume(struct device *dev)
 		return ret;
 
 	if (mode == TPS_MODE_PTCH) {
-		ret = tps25750_apply_patch(tps);
+		ret = tps25750_init(tps);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
index d2c65387994f..0344b65cd55a 100644
--- a/drivers/usb/typec/tipd/tps6598x.h
+++ b/drivers/usb/typec/tipd/tps6598x.h
@@ -213,4 +213,7 @@
 #define TPS_PD_STATUS_PORT_TYPE_SOURCE		2
 #define TPS_PD_STATUS_PORT_TYPE_SOURCE_SINK	3
 
+/* SLEEP CONF REG */
+#define TPS_SLEEP_CONF_SLEEP_MODE_ALLOWED	BIT(0)
+
 #endif /* __TPS6598X_H__ */
-- 
2.34.1

[PATCH v5 12/15] USB: typec: Add trace for tps25750 irq

[Abdel Alkuor]

From: Abdel Alkuor <abdelalkuor@geotab.com>

tps25750 event1 register doesn't have all bits in tps6598x
event registers, only show the events that are masked

Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
---
 drivers/usb/typec/tipd/core.c  |  6 +++++-
 drivers/usb/typec/tipd/trace.h | 22 ++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 3d9877551160..8d6cb67898a5 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -574,7 +574,11 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 		dev_err(tps->dev, "%s: failed to read events\n", __func__);
 		goto err_unlock;
 	}
-	trace_tps6598x_irq(event[0], event[1]);
+
+	if (tps->is_tps25750)
+		trace_tps25750_irq(event[0]);
+	else
+		trace_tps6598x_irq(event[0], event[1]);
 
 	if (!(event[0] | event[1]))
 		goto err_unlock;
diff --git a/drivers/usb/typec/tipd/trace.h b/drivers/usb/typec/tipd/trace.h
index 12cad1bde7cc..28725234a2d8 100644
--- a/drivers/usb/typec/tipd/trace.h
+++ b/drivers/usb/typec/tipd/trace.h
@@ -74,6 +74,13 @@
 		{ APPLE_CD_REG_INT_DATA_STATUS_UPDATE,		"DATA_STATUS_UPDATE" }, \
 		{ APPLE_CD_REG_INT_STATUS_UPDATE,		"STATUS_UPDATE" })
 
+#define show_tps25750_irq_flags(flags) \
+	__print_flags_u64(flags, "|", \
+		{ TPS_REG_INT_PLUG_EVENT,			"PLUG_EVENT" }, \
+		{ TPS_REG_INT_POWER_STATUS_UPDATE,		"POWER_STATUS_UPDATE" }, \
+		{ TPS_REG_INT_STATUS_UPDATE,			"STATUS_UPDATE" }, \
+		{ TPS_REG_INT_PD_STATUS_UPDATE,			"PD_STATUS_UPDATE" })
+
 #define TPS6598X_STATUS_FLAGS_MASK (GENMASK(31, 0) ^ (TPS_STATUS_CONN_STATE_MASK | \
 						      TPS_STATUS_PP_5V0_SWITCH_MASK | \
 						      TPS_STATUS_PP_HV_SWITCH_MASK | \
@@ -230,6 +237,21 @@ TRACE_EVENT(cd321x_irq,
 		      show_cd321x_irq_flags(__entry->event))
 );
 
+TRACE_EVENT(tps25750_irq,
+	    TP_PROTO(u64 event),
+	    TP_ARGS(event),
+
+	    TP_STRUCT__entry(
+			     __field(u64, event)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->event = event;
+			   ),
+
+	    TP_printk("event=%s", show_tps25750_irq_flags(__entry->event))
+);
+
 TRACE_EVENT(tps6598x_status,
 	    TP_PROTO(u32 status),
 	    TP_ARGS(status),
-- 
2.34.1

[PATCH v5 13/15] USB: typec: Add power status trace for tps25750

[Abdel Alkuor]

From: Abdel Alkuor <abdelalkuor@geotab.com>

tps25750 power status register is a subset of tps6598x power status
register.

Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
---
 drivers/usb/typec/tipd/core.c     |  6 +++++-
 drivers/usb/typec/tipd/tps6598x.h | 19 ++++++++++++++++++
 drivers/usb/typec/tipd/trace.h    | 33 +++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 8d6cb67898a5..dd3c76b57aaa 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -484,7 +484,11 @@ static bool tps6598x_read_power_status(struct tps6598x *tps)
 		return false;
 	}
 	tps->pwr_status = pwr_status;
-	trace_tps6598x_power_status(pwr_status);
+
+	if (tps->is_tps25750)
+		trace_tps25750_power_status(pwr_status);
+	else
+		trace_tps6598x_power_status(pwr_status);
 
 	return true;
 }
diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
index 0344b65cd55a..bab6b0f026fc 100644
--- a/drivers/usb/typec/tipd/tps6598x.h
+++ b/drivers/usb/typec/tipd/tps6598x.h
@@ -161,6 +161,25 @@
 #define TPS_POWER_STATUS_BC12_STATUS_CDP 2
 #define TPS_POWER_STATUS_BC12_STATUS_DCP 3
 
+/* TPS25750_REG_POWER_STATUS bits */
+#define TPS25750_POWER_STATUS_CHARGER_DETECT_STATUS_MASK	GENMASK(7, 4)
+#define TPS25750_POWER_STATUS_CHARGER_DETECT_STATUS(p) \
+	TPS_FIELD_GET(TPS25750_POWER_STATUS_CHARGER_DETECT_STATUS_MASK, (p))
+#define TPS25750_POWER_STATUS_CHARGER_ADVERTISE_STATUS_MASK	GENMASK(9, 8)
+#define TPS25750_POWER_STATUS_CHARGER_ADVERTISE_STATUS(p) \
+	TPS_FIELD_GET(TPS25750_POWER_STATUS_CHARGER_ADVERTISE_STATUS_MASK, (p))
+
+#define TPS25750_POWER_STATUS_CHARGER_DET_STATUS_DISABLED	0
+#define TPS25750_POWER_STATUS_CHARGER_DET_STATUS_IN_PROGRESS	1
+#define TPS25750_POWER_STATUS_CHARGER_DET_STATUS_NONE		2
+#define TPS25750_POWER_STATUS_CHARGER_DET_STATUS_SPD		3
+#define TPS25750_POWER_STATUS_CHARGER_DET_STATUS_BC_1_2_CPD	4
+#define TPS25750_POWER_STATUS_CHARGER_DET_STATUS_BC_1_2_DPD	5
+#define TPS25750_POWER_STATUS_CHARGER_DET_STATUS_DIV_1_DCP	6
+#define TPS25750_POWER_STATUS_CHARGER_DET_STATUS_DIV_2_DCP	7
+#define TPS25750_POWER_STATUS_CHARGER_DET_STATUS_DIV_3_DCP	8
+#define TPS25750_POWER_STATUS_CHARGER_DET_STATUS_1_2V_DCP	9
+
 /* TPS_REG_DATA_STATUS bits */
 #define TPS_DATA_STATUS_DATA_CONNECTION	     BIT(0)
 #define TPS_DATA_STATUS_UPSIDE_DOWN	     BIT(1)
diff --git a/drivers/usb/typec/tipd/trace.h b/drivers/usb/typec/tipd/trace.h
index 28725234a2d8..739b0a2a867d 100644
--- a/drivers/usb/typec/tipd/trace.h
+++ b/drivers/usb/typec/tipd/trace.h
@@ -166,6 +166,19 @@
 		{ TPS_POWER_STATUS_BC12_STATUS_CDP, "cdp" }, \
 		{ TPS_POWER_STATUS_BC12_STATUS_SDP, "sdp" })
 
+#define show_tps25750_power_status_charger_detect_status(power_status) \
+	__print_symbolic(TPS25750_POWER_STATUS_CHARGER_DETECT_STATUS(power_status), \
+		{ TPS25750_POWER_STATUS_CHARGER_DET_STATUS_DISABLED,	"disabled"}, \
+		{ TPS25750_POWER_STATUS_CHARGER_DET_STATUS_IN_PROGRESS,	"in progress"}, \
+		{ TPS25750_POWER_STATUS_CHARGER_DET_STATUS_NONE,	"none"}, \
+		{ TPS25750_POWER_STATUS_CHARGER_DET_STATUS_SPD,		"spd"}, \
+		{ TPS25750_POWER_STATUS_CHARGER_DET_STATUS_BC_1_2_CPD,	"cpd"}, \
+		{ TPS25750_POWER_STATUS_CHARGER_DET_STATUS_BC_1_2_DPD,	"dpd"}, \
+		{ TPS25750_POWER_STATUS_CHARGER_DET_STATUS_DIV_1_DCP,	"divider 1 dcp"}, \
+		{ TPS25750_POWER_STATUS_CHARGER_DET_STATUS_DIV_2_DCP,	"divider 2 dcp"}, \
+		{ TPS25750_POWER_STATUS_CHARGER_DET_STATUS_DIV_3_DCP,	"divider 3 dpc"}, \
+		{ TPS25750_POWER_STATUS_CHARGER_DET_STATUS_1_2V_DCP,	"1.2V dpc"})
+
 #define TPS_DATA_STATUS_FLAGS_MASK (GENMASK(31, 0) ^ (TPS_DATA_STATUS_DP_PIN_ASSIGNMENT_MASK | \
 						      TPS_DATA_STATUS_TBT_CABLE_SPEED_MASK | \
 						      TPS_DATA_STATUS_TBT_CABLE_GEN_MASK))
@@ -299,6 +312,26 @@ TRACE_EVENT(tps6598x_power_status,
 		    )
 );
 
+TRACE_EVENT(tps25750_power_status,
+	    TP_PROTO(u16 power_status),
+	    TP_ARGS(power_status),
+
+	    TP_STRUCT__entry(
+			     __field(u16, power_status)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->power_status = power_status;
+			   ),
+
+	    TP_printk("conn: %d, pwr-role: %s, typec: %s, charger detect: %s",
+		      !!TPS_POWER_STATUS_CONNECTION(__entry->power_status),
+		      show_power_status_source_sink(__entry->power_status),
+		      show_power_status_typec_status(__entry->power_status),
+		      show_tps25750_power_status_charger_detect_status(__entry->power_status)
+		    )
+);
+
 TRACE_EVENT(tps6598x_data_status,
 	    TP_PROTO(u32 data_status),
 	    TP_ARGS(data_status),
-- 
2.34.1

[PATCH v5 14/15] USB: typec: Add status trace for tps25750

[Abdel Alkuor]

From: Abdel Alkuor <abdelalkuor@geotab.com>

tps25750 status register is a subset of tps6598x status register, hence
a trace for tps25750 status register is added.

Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
---
 drivers/usb/typec/tipd/core.c  | 10 +++++----
 drivers/usb/typec/tipd/trace.h | 37 ++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index dd3c76b57aaa..326c23bfa8e6 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -453,7 +453,11 @@ static bool tps6598x_read_status(struct tps6598x *tps, u32 *status)
 		dev_err(tps->dev, "%s: failed to read status\n", __func__);
 		return false;
 	}
-	trace_tps6598x_status(*status);
+
+	if (tps->is_tps25750)
+		trace_tps25750_status(*status);
+	else
+		trace_tps6598x_status(*status);
 
 	return true;
 }
@@ -1188,10 +1192,8 @@ static int tps6598x_probe(struct i2c_client *client)
 	if (ret)
 		goto err_reset_controller;
 
-	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
-	if (ret < 0)
+	if (!tps6598x_read_status(tps, &status))
 		goto err_clear_mask;
-	trace_tps6598x_status(status);
 
 	/*
 	 * This fwnode has a "compatible" property, but is never populated as a
diff --git a/drivers/usb/typec/tipd/trace.h b/drivers/usb/typec/tipd/trace.h
index 739b0a2a867d..afa0875a9de5 100644
--- a/drivers/usb/typec/tipd/trace.h
+++ b/drivers/usb/typec/tipd/trace.h
@@ -91,6 +91,14 @@
 						      TPS_STATUS_USB_HOST_PRESENT_MASK | \
 						      TPS_STATUS_LEGACY_MASK))
 
+#define TPS25750_STATUS_FLAGS_MASK (GENMASK(31, 0) ^ (TPS_STATUS_CONN_STATE_MASK | \
+						      GENMASK(19, 7) | \
+						      TPS_STATUS_VBUS_STATUS_MASK | \
+						      TPS_STATUS_USB_HOST_PRESENT_MASK | \
+						      TPS_STATUS_LEGACY_MASK | \
+						      BIT(26) | \
+						      GENMASK(31, 28)))
+
 #define show_status_conn_state(status) \
 	__print_symbolic(TPS_STATUS_CONN_STATE((status)), \
 		{ TPS_STATUS_CONN_STATE_CONN_WITH_R_A,	"conn-Ra"  }, \
@@ -148,6 +156,14 @@
 		      { TPS_STATUS_HIGH_VOLAGE_WARNING,	"HIGH_VOLAGE_WARNING" }, \
 		      { TPS_STATUS_HIGH_LOW_VOLTAGE_WARNING, "HIGH_LOW_VOLTAGE_WARNING" })
 
+#define show_tps25750_status_flags(flags) \
+	__print_flags((flags & TPS25750_STATUS_FLAGS_MASK), "|", \
+		      { TPS_STATUS_PLUG_PRESENT,	"PLUG_PRESENT" }, \
+		      { TPS_STATUS_PLUG_UPSIDE_DOWN,	"UPSIDE_DOWN" }, \
+		      { TPS_STATUS_PORTROLE,		"PORTROLE" }, \
+		      { TPS_STATUS_DATAROLE,		"DATAROLE" }, \
+		      { TPS_STATUS_BIST,		"BIST" })
+
 #define show_power_status_source_sink(power_status) \
 	__print_symbolic(TPS_POWER_STATUS_SOURCESINK(power_status), \
 		{ 1, "sink" }, \
@@ -292,6 +308,27 @@ TRACE_EVENT(tps6598x_status,
 		    )
 );
 
+TRACE_EVENT(tps25750_status,
+	    TP_PROTO(u32 status),
+	    TP_ARGS(status),
+
+	    TP_STRUCT__entry(
+			     __field(u32, status)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->status = status;
+			   ),
+
+	    TP_printk("conn: %s, vbus: %s, usb-host: %s, legacy: %s, flags: %s",
+		      show_status_conn_state(__entry->status),
+		      show_status_vbus_status(__entry->status),
+		      show_status_usb_host_present(__entry->status),
+		      show_status_legacy(__entry->status),
+		      show_tps25750_status_flags(__entry->status)
+		    )
+);
+
 TRACE_EVENT(tps6598x_power_status,
 	    TP_PROTO(u16 power_status),
 	    TP_ARGS(power_status),
-- 
2.34.1

[PATCH v5 15/15] USB: typec: Do not check VID for tps25750

[Abdel Alkuor]

From: Abdel Alkuor <abdelalkuor@geotab.com>

tps25750 doesn't have VID register, check VID for PD controllers
other than tps25750

Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
---
 drivers/usb/typec/tipd/core.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 326c23bfa8e6..c1399e12a170 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -1142,10 +1142,6 @@ static int tps6598x_probe(struct i2c_client *client)
 	if (IS_ERR(tps->regmap))
 		return PTR_ERR(tps->regmap);
 
-	ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
-	if (ret < 0 || !vid)
-		return -ENODEV;
-
 	/*
 	 * Checking can the adapter handle SMBus protocol. If it can not, the
 	 * driver needs to take care of block reads separately.
@@ -1176,6 +1172,12 @@ static int tps6598x_probe(struct i2c_client *client)
 
 	tps->irq_handler = irq_handler;
 
+	if (!tps->is_tps25750) {
+		ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
+		if (ret < 0 || !vid)
+			return -ENODEV;
+	}
+
 	/* Make sure the controller has application firmware running */
 	ret = tps6598x_check_mode(tps, &mode);
 	if (ret)
-- 
2.34.1

Re: [PATCH v5 04/15] USB: typec: Load TPS25750 patch bundle

[kernel test robot]

Hi Abdel,

kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on linus/master v6.6-rc1 next-20230915]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Abdel-Alkuor/dt-bindings-usb-tps6598x-Add-tps25750/20230917-233037
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20230917152639.21443-5-alkuor%40gmail.com
patch subject: [PATCH v5 04/15] USB: typec: Load TPS25750 patch bundle
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230918/202309180034.fV0JkOAu-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230918/202309180034.fV0JkOAu-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309180034.fV0JkOAu-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/device.h:15,
                    from include/linux/acpi.h:14,
                    from include/linux/i2c.h:13,
                    from drivers/usb/typec/tipd/core.c:9:
   drivers/usb/typec/tipd/core.c: In function 'tps25750_start_patch_burst_mode':
>> drivers/usb/typec/tipd/core.c:844:35: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
     844 |                 dev_err(tps->dev, "Failed to write patch %s of %lu bytes\n",
         |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ^~~
   include/linux/dev_printk.h:144:56: note: in expansion of macro 'dev_fmt'
     144 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                        ^~~~~~~
   drivers/usb/typec/tipd/core.c:844:17: note: in expansion of macro 'dev_err'
     844 |                 dev_err(tps->dev, "Failed to write patch %s of %lu bytes\n",
         |                 ^~~~~~~
   drivers/usb/typec/tipd/core.c:844:66: note: format string is defined here
     844 |                 dev_err(tps->dev, "Failed to write patch %s of %lu bytes\n",
         |                                                                ~~^
         |                                                                  |
         |                                                                  long unsigned int
         |                                                                %u


vim +844 drivers/usb/typec/tipd/core.c

   801	
   802	static int tps25750_start_patch_burst_mode(struct tps6598x *tps)
   803	{
   804		int ret;
   805		const struct firmware *fw;
   806		const char *firmware_name;
   807		struct {
   808			u32 fw_size;
   809			u8 addr;
   810			u8 timeout;
   811		} __packed bpms_data;
   812	
   813		ret = device_property_read_string(tps->dev, "firmware-name",
   814						  &firmware_name);
   815		if (ret)
   816			return ret;
   817	
   818		ret = request_firmware(&fw, firmware_name, tps->dev);
   819		if (ret) {
   820			dev_err(tps->dev, "failed to retrieve \"%s\"\n", firmware_name);
   821			return ret;
   822		}
   823	
   824		if (fw->size == 0) {
   825			ret = -EINVAL;
   826			goto release_fw;
   827		}
   828	
   829		ret = device_property_read_u8(tps->dev, "ti,patch-address", &bpms_data.addr);
   830		if (ret) {
   831			dev_err(tps->dev, "failed to get patch address\n");
   832			return ret;
   833		}
   834	
   835		bpms_data.fw_size = fw->size;
   836		bpms_data.timeout = TPS_BUNDLE_TIMEOUT;
   837	
   838		ret = tps25750_exec_pbms(tps, (u8 *)&bpms_data, sizeof(bpms_data));
   839		if (ret)
   840			goto release_fw;
   841	
   842		ret = tps25750_write_firmware(tps, bpms_data.addr, fw->data, fw->size);
   843		if (ret) {
 > 844			dev_err(tps->dev, "Failed to write patch %s of %lu bytes\n",
   845				firmware_name, fw->size);
   846			goto release_fw;
   847		}
   848	
   849		/*
   850		 * A delay of 500us is required after the firmware is written
   851		 * based on pg.62 in tps6598x Host Interface Technical
   852		 * Reference Manual
   853		 * https://www.ti.com/lit/ug/slvuc05a/slvuc05a.pdf
   854		 */
   855		udelay(500);
   856	
   857	release_fw:
   858		release_firmware(fw);
   859	
   860		return ret;
   861	}
   862	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v5 04/15] USB: typec: Load TPS25750 patch bundle

[kernel test robot]

Hi Abdel,

kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.6-rc1 next-20230915]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Abdel-Alkuor/dt-bindings-usb-tps6598x-Add-tps25750/20230917-233037
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20230917152639.21443-5-alkuor%40gmail.com
patch subject: [PATCH v5 04/15] USB: typec: Load TPS25750 patch bundle
config: i386-buildonly-randconfig-006-20230917 (https://download.01.org/0day-ci/archive/20230918/202309180124.ZkZ5E7oC-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230918/202309180124.ZkZ5E7oC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309180124.ZkZ5E7oC-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/device.h:15,
                    from include/linux/acpi.h:14,
                    from include/linux/i2c.h:13,
                    from drivers/usb/typec/tipd/core.c:9:
   drivers/usb/typec/tipd/core.c: In function 'tps25750_start_patch_burst_mode':
>> drivers/usb/typec/tipd/core.c:844:21: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'const unsigned int'} [-Wformat=]
     844 |   dev_err(tps->dev, "Failed to write patch %s of %lu bytes\n",
         |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:110:16: note: in definition of macro 'dev_printk_index_wrap'
     110 |   _p_func(dev, fmt, ##__VA_ARGS__);   \
         |                ^~~
   include/linux/dev_printk.h:144:49: note: in expansion of macro 'dev_fmt'
     144 |  dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                 ^~~~~~~
   drivers/usb/typec/tipd/core.c:844:3: note: in expansion of macro 'dev_err'
     844 |   dev_err(tps->dev, "Failed to write patch %s of %lu bytes\n",
         |   ^~~~~~~
   drivers/usb/typec/tipd/core.c:844:52: note: format string is defined here
     844 |   dev_err(tps->dev, "Failed to write patch %s of %lu bytes\n",
         |                                                  ~~^
         |                                                    |
         |                                                    long unsigned int
         |                                                  %u


vim +844 drivers/usb/typec/tipd/core.c

   801	
   802	static int tps25750_start_patch_burst_mode(struct tps6598x *tps)
   803	{
   804		int ret;
   805		const struct firmware *fw;
   806		const char *firmware_name;
   807		struct {
   808			u32 fw_size;
   809			u8 addr;
   810			u8 timeout;
   811		} __packed bpms_data;
   812	
   813		ret = device_property_read_string(tps->dev, "firmware-name",
   814						  &firmware_name);
   815		if (ret)
   816			return ret;
   817	
   818		ret = request_firmware(&fw, firmware_name, tps->dev);
   819		if (ret) {
   820			dev_err(tps->dev, "failed to retrieve \"%s\"\n", firmware_name);
   821			return ret;
   822		}
   823	
   824		if (fw->size == 0) {
   825			ret = -EINVAL;
   826			goto release_fw;
   827		}
   828	
   829		ret = device_property_read_u8(tps->dev, "ti,patch-address", &bpms_data.addr);
   830		if (ret) {
   831			dev_err(tps->dev, "failed to get patch address\n");
   832			return ret;
   833		}
   834	
   835		bpms_data.fw_size = fw->size;
   836		bpms_data.timeout = TPS_BUNDLE_TIMEOUT;
   837	
   838		ret = tps25750_exec_pbms(tps, (u8 *)&bpms_data, sizeof(bpms_data));
   839		if (ret)
   840			goto release_fw;
   841	
   842		ret = tps25750_write_firmware(tps, bpms_data.addr, fw->data, fw->size);
   843		if (ret) {
 > 844			dev_err(tps->dev, "Failed to write patch %s of %lu bytes\n",
   845				firmware_name, fw->size);
   846			goto release_fw;
   847		}
   848	
   849		/*
   850		 * A delay of 500us is required after the firmware is written
   851		 * based on pg.62 in tps6598x Host Interface Technical
   852		 * Reference Manual
   853		 * https://www.ti.com/lit/ug/slvuc05a/slvuc05a.pdf
   854		 */
   855		udelay(500);
   856	
   857	release_fw:
   858		release_firmware(fw);
   859	
   860		return ret;
   861	}
   862	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v5 01/15] dt-bindings: usb: tps6598x: Add tps25750

[Krzysztof Kozlowski]

On 17/09/2023 17:26, Abdel Alkuor wrote:
> From: Abdel Alkuor <abdelalkuor@geotab.com>
> 
> TPS25750 is USB TypeC PD controller which is a subset of TPS6598x.
> 
> Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> ---
>  .../devicetree/bindings/usb/ti,tps6598x.yaml  | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> index 5497a60cddbc..e49bd92b5276 100644
> --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> @@ -20,6 +20,8 @@ properties:
>      enum:
>        - ti,tps6598x
>        - apple,cd321x
> +      - ti,tps25750
> +
>    reg:
>      maxItems: 1
>  
> @@ -32,10 +34,45 @@ properties:
>      items:
>        - const: irq
>  
> +  firmware-name:
> +    description: |
> +      Should contain the name of the default patch binary
> +      file located on the firmware search path which is
> +      used to switch the controller into APP mode.
> +      This is used when tps25750 doesn't have an EEPROM
> +      connected to it.
> +    maxItems: 1
> +
> +  ti,patch-address:
> +    description: |
> +      One of PBMs command data field is I2C slave address
> +      which is used when writing the patch for TPS25750.
> +      The slave address can be any value except 0x00, 0x20,
> +      0x21, 0x22, and 0x23

Why this cannot be another entry in the reg?

> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    minimum: 1
> +    maximum: 0x7e
> +
>  required:
>    - compatible
>    - reg
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: ti,tps25750
> +    then:
> +      required:
> +        - ti,patch-address
> +        - connector


Why? Connector should be required or not required for both devices. What
is different between them?


Best regards,
Krzysztof

Re: [PATCH v5 11/15] USB: typec: Enable sleep mode for tps25750

[Krzysztof Kozlowski]

On 17/09/2023 17:26, Abdel Alkuor wrote:
> From: Abdel Alkuor <abdelalkuor@geotab.com>
> 
> Allow controller to enter sleep mode after the device
> is idle for sleep time.
> 
> Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> ---
>  drivers/usb/typec/tipd/core.c     | 29 ++++++++++++++++++++++++++++-
>  drivers/usb/typec/tipd/tps6598x.h |  3 +++
>  2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index a97fda68cb54..3d9877551160 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -42,6 +42,7 @@
>  #define TPS_REG_PD_STATUS		0x40
>  #define TPS_REG_RX_IDENTITY_SOP		0x48
>  #define TPS_REG_DATA_STATUS		0x5f
> +#define TPS_REG_SLEEP_CONF		0x70
>  
>  /* TPS_REG_SYSTEM_CONF bits */
>  #define TPS_SYSCONF_PORTINFO(c)		((c) & 7)
> @@ -205,6 +206,11 @@ static inline int tps6598x_read64(struct tps6598x *tps, u8 reg, u64 *val)
>  	return tps6598x_block_read(tps, reg, val, sizeof(u64));
>  }
>  
> +static inline int tps6598x_write8(struct tps6598x *tps, u8 reg, u8 val)
> +{
> +	return tps6598x_block_write(tps, reg, &val, sizeof(u8));
> +}
> +
>  static inline int tps6598x_write64(struct tps6598x *tps, u8 reg, u64 val)
>  {
>  	return tps6598x_block_write(tps, reg, &val, sizeof(u64));
> @@ -977,6 +983,24 @@ static int tps25750_apply_patch(struct tps6598x *tps)
>  	return 0;
>  };
>  
> +static int tps25750_init(struct tps6598x *tps)
> +{
> +	int ret;
> +
> +	ret = tps25750_apply_patch(tps);
> +	if (ret)
> +		return ret;
> +
> +	ret = tps6598x_write8(tps, TPS_REG_SLEEP_CONF,
> +			      TPS_SLEEP_CONF_SLEEP_MODE_ALLOWED);
> +	if (ret)
> +		dev_warn(tps->dev,
> +			 "%s: failed to enable sleep mode: %d\n",
> +			 __func__, ret);
> +
> +	return 0;
> +}
> +
>  static int
>  tps6598x_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
>  {
> @@ -1131,6 +1155,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  		irq_handler = cd321x_interrupt;
>  	} else {
>  		tps->is_tps25750 = of_device_is_compatible(np, "ti,tps25750");
> +
>  		/* Enable power status, data status and plug event interrupts */
>  		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
>  			TPS_REG_INT_DATA_STATUS_UPDATE |
> @@ -1138,6 +1163,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  	}
>  
>  	tps->irq_handler = irq_handler;
> +

Drop

>  	/* Make sure the controller has application firmware running */
>  	ret = tps6598x_check_mode(tps, &mode);
>  	if (ret)
> @@ -1149,6 +1175,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  			return ret;
>  	}
>  
> +

That's not really relevant, neither correct.

>  	ret = tps6598x_write64(tps, TPS_REG_INT_MASK1, mask1);
>  	if (ret)
>  		goto err_reset_controller;
> @@ -1286,7 +1313,7 @@ static int __maybe_unused tps6598x_resume(struct device *dev)
>  		return ret;

Best regards,
Krzysztof

Re: [PATCH v5 01/15] dt-bindings: usb: tps6598x: Add tps25750

[Abdel Alkuo]

On Sun, Sep 17, 2023 at 07:30:52PM +0200, Krzysztof Kozlowski wrote:
> On 17/09/2023 17:26, Abdel Alkuor wrote:
> > From: Abdel Alkuor <abdelalkuor@geotab.com>
> > 
> > TPS25750 is USB TypeC PD controller which is a subset of TPS6598x.
> > 
> > Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> > ---
> >  .../devicetree/bindings/usb/ti,tps6598x.yaml  | 70 +++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> > index 5497a60cddbc..e49bd92b5276 100644
> > --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> > +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> > @@ -20,6 +20,8 @@ properties:
> >      enum:
> >        - ti,tps6598x
> >        - apple,cd321x
> > +      - ti,tps25750
> > +
> >    reg:
> >      maxItems: 1
> >  
> > @@ -32,10 +34,45 @@ properties:
> >      items:
> >        - const: irq
> >  
> > +  firmware-name:
> > +    description: |
> > +      Should contain the name of the default patch binary
> > +      file located on the firmware search path which is
> > +      used to switch the controller into APP mode.
> > +      This is used when tps25750 doesn't have an EEPROM
> > +      connected to it.
> > +    maxItems: 1
> > +
> > +  ti,patch-address:
> > +    description: |
> > +      One of PBMs command data field is I2C slave address
> > +      which is used when writing the patch for TPS25750.
> > +      The slave address can be any value except 0x00, 0x20,
> > +      0x21, 0x22, and 0x23
> 
> Why this cannot be another entry in the reg?
> 
This address is different than the physical address of PD controller.
The patch address will be used instead of PD controller address when
writing the patch. I thought reg proprity is only for a device physical
address, should I add another entry in the reg property in this case?
> > +    $ref: /schemas/types.yaml#/definitions/uint8
> > +    minimum: 1
> > +    maximum: 0x7e
> > +
> >  required:
> >    - compatible
> >    - reg
> >  
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: ti,tps25750
> > +    then:
> > +      required:
> > +        - ti,patch-address
> > +        - connector
> 
> 
> Why? Connector should be required or not required for both devices. What
> is different between them?
> 
The data-role for tps6598x can be extracted from system config register
which it doesn't exist in tps25750, so the only way to extract
this information is by using data-role property from the Connector for
tps25750, hence Connector and data-role are set as required for
tps25750.
> 
> Best regards,
> Krzysztof
>
Thanks,
Abdel

Re: [PATCH v5 02/15] USB: typec: Add cmd timeout and response delay

[Heikki Krogerus]

On Sun, Sep 17, 2023 at 11:26:26AM -0400, Abdel Alkuor wrote:
> Some commands in tps25750 take longer than 1 second
> to complete, and some responses need some delay before
> the result becomes available.
> 
> Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> ---
>  drivers/usb/typec/tipd/core.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 37b56ce75f39..a8aee4e1aeba 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -284,7 +284,8 @@ static void tps6598x_disconnect(struct tps6598x *tps, u32 status)
>  
>  static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
>  			     size_t in_len, u8 *in_data,
> -			     size_t out_len, u8 *out_data)
> +			     size_t out_len, u8 *out_data,
> +			     u32 cmd_timeout_ms, u32 res_delay_ms)

It looks like 1s/0s is still the "default", so you could have just
made this old function a wrapper:

static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
			     size_t in_len, u8 *in_data,
			     size_t out_len, u8 *out_data)
{
        return tps6598x_exec_cmd_tmo(tps, cmd, in_len, in_data, out_len, out_data, 1000, 0);
}

thanks,

-- 
heikki

Re: [PATCH v5 03/15] USB: typec: Add patch mode to tps6598x

[Heikki Krogerus]

On Sun, Sep 17, 2023 at 11:26:27AM -0400, Abdel Alkuor wrote:
> TPS25750 has a patch mode indicating the device requires
> a configuration to get the device into operational mode
> 
> Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> ---
>  drivers/usb/typec/tipd/core.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index a8aee4e1aeba..6d2151325fbb 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -68,6 +68,7 @@ enum {
>  	TPS_MODE_BOOT,
>  	TPS_MODE_BIST,
>  	TPS_MODE_DISC,
> +	TPS_MODE_PTCH,
>  };
>  
>  static const char *const modes[] = {
> @@ -75,6 +76,7 @@ static const char *const modes[] = {
>  	[TPS_MODE_BOOT]	= "BOOT",
>  	[TPS_MODE_BIST]	= "BIST",
>  	[TPS_MODE_DISC]	= "DISC",
> +	[TPS_MODE_PTCH] = "PTCH",
>  };
>  
>  /* Unrecognized commands will be replaced with "!CMD" */
> @@ -576,7 +578,7 @@ static void tps6598x_poll_work(struct work_struct *work)
>  			   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
>  }
>  
> -static int tps6598x_check_mode(struct tps6598x *tps)
> +static int tps6598x_check_mode(struct tps6598x *tps, u8 *curr_mode)
>  {
>  	char mode[5] = { };
>  	int ret;
> @@ -585,8 +587,11 @@ static int tps6598x_check_mode(struct tps6598x *tps)
>  	if (ret)
>  		return ret;
>  
> -	switch (match_string(modes, ARRAY_SIZE(modes), mode)) {
> +	*curr_mode = match_string(modes, ARRAY_SIZE(modes), mode);
> +
> +	switch (*curr_mode) {
>  	case TPS_MODE_APP:
> +	case TPS_MODE_PTCH:

This check is OK, but it seems that you are not using that curr_mode
for anything yet, so don't change the paramaters of this function in
this patch.

I would also just return the mode. Then this function would be used
like this:

        ret = tps6598x_check_mode(...)
        if (ret < 0)
                return ret;

Now mode = ret.

>  		return 0;
>  	case TPS_MODE_BOOT:
>  		dev_warn(tps->dev, "dead-battery condition\n");
> @@ -715,6 +720,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  	u32 vid;
>  	int ret;
>  	u64 mask1;
> +	u8 mode;
>  
>  	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
>  	if (!tps)
> @@ -759,7 +765,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  
>  	tps->irq_handler = irq_handler;
>  	/* Make sure the controller has application firmware running */
> -	ret = tps6598x_check_mode(tps);
> +	ret = tps6598x_check_mode(tps, &mode);

Actually, if you need to know the mode in here later, then I'm not
sure tps6592x_check_mode() is useful after that. This is the only
place it's called. Just read and check the mode in here directly at
that point.

thanks,

-- 
heikki

Re: [PATCH v5 04/15] USB: typec: Load TPS25750 patch bundle

[Heikki Krogerus]

On Sun, Sep 17, 2023 at 11:26:28AM -0400, Abdel Alkuor wrote:
> From: Abdel Alkuor <abdelalkuor@geotab.com>
> 
> TPS25750 controller requires a binary to be loaded with a configuration
> binary by an EEPROM or a host.
> 
> Appling a patch bundling using a host is implemented based on the flow
> diagram pg.62 in TPS25750 host interface manual.
> https://www.ti.com/lit/ug/slvuc05a/slvuc05a.pdf
> 
> The flow diagram can be summarized as following:
> - Start the patch loading sequence with patch bundle information by
>   executing PBMs
> - Write the whole patch at once
> - When writing the patch fails, execute PBMe which instructs the PD controller
>   to end the patching process
> - After writing the patch successfully, execute PBMc which verifies the patch
>   integrity and applies the patch internally
> - Wait for the device to switch into APP mode (normal operation)
> 
> The execuation flow diagram polls the events register and then polls the
> corresponding register related to the event as well before advancing to the next
> state. Polling the events register is a redundant step, in this implementation
> only the corresponding register related to the event is polled.
> 
> Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> ---
>  drivers/usb/typec/tipd/core.c | 237 +++++++++++++++++++++++++++++++++-
>  1 file changed, 236 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 6d2151325fbb..fea139c72d6d 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -17,6 +17,7 @@
>  #include <linux/usb/typec_altmode.h>
>  #include <linux/usb/role.h>
>  #include <linux/workqueue.h>
> +#include <linux/firmware.h>
>  
>  #include "tps6598x.h"
>  #include "trace.h"
> @@ -43,6 +44,23 @@
>  /* TPS_REG_SYSTEM_CONF bits */
>  #define TPS_SYSCONF_PORTINFO(c)		((c) & 7)
>  
> +/*
> + * BPMs task timeout, recommended 5 seconds
> + * pg.48 TPS2575 Host Interface Technical Reference
> + * Manual (Rev. A)
> + * https://www.ti.com/lit/ug/slvuc05a/slvuc05a.pdf
> + */
> +#define TPS_BUNDLE_TIMEOUT	0x32
> +
> +/* BPMs return code */
> +#define TPS_TASK_BPMS_INVALID_BUNDLE_SIZE	0x4
> +#define TPS_TASK_BPMS_INVALID_SLAVE_ADDR	0x5
> +#define TPS_TASK_BPMS_INVALID_TIMEOUT		0x6
> +
> +/* PBMc data out */
> +#define TPS_PBMC_RC	0 /* Return code */
> +#define TPS_PBMC_DPCS	2 /* device patch complete status */
> +
>  enum {
>  	TPS_PORTINFO_SINK,
>  	TPS_PORTINFO_SINK_ACCESSORY,
> @@ -88,6 +106,8 @@ struct tps6598x {
>  	struct mutex lock; /* device lock */
>  	u8 i2c_protocol:1;
>  
> +	u8 is_tps25750:1;
> +
>  	struct typec_port *port;
>  	struct typec_partner *partner;
>  	struct usb_pd_identity partner_identity;
> @@ -708,6 +728,203 @@ static int devm_tps6598_psy_register(struct tps6598x *tps)
>  	return PTR_ERR_OR_ZERO(tps->psy);
>  }
>  
> +static int
> +tps25750_write_firmware(struct tps6598x *tps,
> +			u8 bpms_addr, const u8 *data, size_t len)
> +{
> +	struct i2c_client *client = to_i2c_client(tps->dev);
> +	int ret;
> +	u8 slave_addr;
> +	int timeout;
> +
> +	slave_addr = client->addr;
> +	timeout = client->adapter->timeout;
> +
> +	/*
> +	 * binary configuration size is around ~16Kbytes
> +	 * which might take some time to finish writing it
> +	 */
> +	client->adapter->timeout = msecs_to_jiffies(5000);
> +	client->addr = bpms_addr;
> +
> +	ret = regmap_raw_write(tps->regmap, data[0], &data[1], len - 1);
> +
> +	client->addr = slave_addr;
> +	client->adapter->timeout = timeout;
> +
> +	return ret;
> +}
> +
> +static int
> +tps25750_exec_pbms(struct tps6598x *tps, u8 *in_data, size_t in_len)
> +{
> +	int ret;
> +	u8 rc;
> +
> +	ret = tps6598x_exec_cmd(tps, "PBMs", in_len, in_data,
> +				sizeof(rc), &rc, 4000, 0);
> +	if (ret)
> +		return ret;
> +
> +	switch (rc) {
> +	case TPS_TASK_BPMS_INVALID_BUNDLE_SIZE:
> +		dev_err(tps->dev, "%s: invalid fw size\n", __func__);
> +		return -EINVAL;
> +	case TPS_TASK_BPMS_INVALID_SLAVE_ADDR:
> +		dev_err(tps->dev, "%s: invalid slave address\n", __func__);
> +		return -EINVAL;
> +	case TPS_TASK_BPMS_INVALID_TIMEOUT:
> +		dev_err(tps->dev, "%s: timed out\n", __func__);
> +		return -ETIMEDOUT;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tps25750_abort_patch_process(struct tps6598x *tps)
> +{
> +	int ret;
> +	u8 mode;
> +
> +	ret = tps6598x_exec_cmd(tps, "PBMe", 0, NULL, 0, NULL, 1000, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = tps6598x_check_mode(tps, &mode);
> +	if (mode != TPS_MODE_PTCH)
> +		dev_err(tps->dev, "failed to switch to \"PTCH\" mode\n");
> +
> +	return ret;
> +}
> +
> +static int tps25750_start_patch_burst_mode(struct tps6598x *tps)
> +{
> +	int ret;
> +	const struct firmware *fw;
> +	const char *firmware_name;
> +	struct {
> +		u32 fw_size;
> +		u8 addr;
> +		u8 timeout;
> +	} __packed bpms_data;
> +
> +	ret = device_property_read_string(tps->dev, "firmware-name",
> +					  &firmware_name);
> +	if (ret)
> +		return ret;
> +
> +	ret = request_firmware(&fw, firmware_name, tps->dev);
> +	if (ret) {
> +		dev_err(tps->dev, "failed to retrieve \"%s\"\n", firmware_name);
> +		return ret;
> +	}
> +
> +	if (fw->size == 0) {
> +		ret = -EINVAL;
> +		goto release_fw;
> +	}
> +
> +	ret = device_property_read_u8(tps->dev, "ti,patch-address", &bpms_data.addr);
> +	if (ret) {
> +		dev_err(tps->dev, "failed to get patch address\n");
> +		return ret;
> +	}
> +
> +	bpms_data.fw_size = fw->size;
> +	bpms_data.timeout = TPS_BUNDLE_TIMEOUT;
> +
> +	ret = tps25750_exec_pbms(tps, (u8 *)&bpms_data, sizeof(bpms_data));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = tps25750_write_firmware(tps, bpms_data.addr, fw->data, fw->size);
> +	if (ret) {
> +		dev_err(tps->dev, "Failed to write patch %s of %lu bytes\n",
> +			firmware_name, fw->size);
> +		goto release_fw;
> +	}
> +
> +	/*
> +	 * A delay of 500us is required after the firmware is written
> +	 * based on pg.62 in tps6598x Host Interface Technical
> +	 * Reference Manual
> +	 * https://www.ti.com/lit/ug/slvuc05a/slvuc05a.pdf
> +	 */
> +	udelay(500);
> +
> +release_fw:
> +	release_firmware(fw);
> +
> +	return ret;
> +}
> +
> +static int tps25750_complete_patch_process(struct tps6598x *tps)
> +{
> +	int ret;
> +	u8 out_data[40];
> +	u8 dummy[2] = { };
> +
> +	/*
> +	 * Without writing something to DATA_IN, this command would
> +	 * return an error
> +	 */
> +	ret = tps6598x_exec_cmd(tps, "PBMc", sizeof(dummy), dummy,
> +				sizeof(out_data), out_data, 2000, 20);
> +	if (ret)
> +		return ret;
> +
> +	if (out_data[TPS_PBMC_RC]) {
> +		dev_err(tps->dev,
> +			"%s: pbmc failed: %u\n", __func__,
> +			out_data[TPS_PBMC_RC]);
> +		return -EIO;
> +	}
> +
> +	if (out_data[TPS_PBMC_DPCS]) {
> +		dev_err(tps->dev,
> +			"%s: failed device patch complete status: %u\n",
> +			__func__, out_data[TPS_PBMC_DPCS]);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tps25750_apply_patch(struct tps6598x *tps)
> +{
> +	int ret;
> +	unsigned long timeout;
> +	u8 mode;
> +
> +	ret = tps25750_start_patch_burst_mode(tps);
> +	if (ret) {
> +		tps25750_abort_patch_process(tps);
> +		return ret;
> +	}
> +
> +	ret = tps25750_complete_patch_process(tps);
> +	if (ret)
> +		return ret;
> +
> +	timeout = jiffies + msecs_to_jiffies(1000);
> +
> +	do {
> +		ret = tps6598x_check_mode(tps, &mode);
> +		if (ret)
> +			return ret;
> +
> +		if (time_is_before_jiffies(timeout))
> +			return -ETIMEDOUT;
> +
> +	} while (mode != TPS_MODE_APP);
> +
> +	dev_info(tps->dev, "controller switched to \"APP\" mode\n");
> +
> +	return 0;
> +};
> +
>  static int tps6598x_probe(struct i2c_client *client)
>  {
>  	irq_handler_t irq_handler = tps6598x_interrupt;
> @@ -757,6 +974,8 @@ static int tps6598x_probe(struct i2c_client *client)
>  
>  		irq_handler = cd321x_interrupt;
>  	} else {
> +
> +		tps->is_tps25750 = of_device_is_compatible(np, "ti,tps25750");
>  		/* Enable power status, data status and plug event interrupts */
>  		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
>  			TPS_REG_INT_DATA_STATUS_UPDATE |
> @@ -769,9 +988,15 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	if (tps->is_tps25750 && mode == TPS_MODE_PTCH) {
> +		ret = tps25750_apply_patch(tps);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = tps6598x_write64(tps, TPS_REG_INT_MASK1, mask1);
>  	if (ret)
> -		return ret;
> +		goto err_reset_controller;
>  
>  	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
>  	if (ret < 0)
> @@ -891,6 +1116,10 @@ static int tps6598x_probe(struct i2c_client *client)
>  	fwnode_handle_put(fwnode);
>  err_clear_mask:
>  	tps6598x_write64(tps, TPS_REG_INT_MASK1, 0);
> +err_reset_controller:
> +	/* Reset PD controller to remove any applied patch */
> +	if (tps->is_tps25750)
> +		tps6598x_exec_cmd(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
>  	return ret;
>  }
>  
> @@ -901,9 +1130,14 @@ static void tps6598x_remove(struct i2c_client *client)
>  	if (!client->irq)
>  		cancel_delayed_work_sync(&tps->wq_poll);
>  
> +	devm_free_irq(tps->dev, client->irq, tps);
>  	tps6598x_disconnect(tps, 0);
>  	typec_unregister_port(tps->port);
>  	usb_role_switch_put(tps->role_sw);
> +
> +	/* Reset PD controller to remove any applied patch */
> +	if (tps->is_tps25750)
> +		tps6598x_exec_cmd(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
>  }
>  
>  static int __maybe_unused tps6598x_suspend(struct device *dev)
> @@ -946,6 +1180,7 @@ static const struct dev_pm_ops tps6598x_pm_ops = {
>  static const struct of_device_id tps6598x_of_match[] = {
>  	{ .compatible = "ti,tps6598x", },
>  	{ .compatible = "apple,cd321x", },
> +	{ .compatible = "ti,tps25750", },

This is probable OK for now, we can mix this stuff into core.c now, but
later I want this driver (core.c) to be converted into a library that
contains only the common functionality. TPS5750 will at that point have
its own probe/glue driver.

>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, tps6598x_of_match);
> -- 
> 2.34.1

thanks,

-- 
heikki

Re: [PATCH v5 05/15] USB: typec: Check for EEPROM present

[Heikki Krogerus]

On Sun, Sep 17, 2023 at 11:26:29AM -0400, Abdel Alkuor wrote:
> From: Abdel Alkuor <abdelalkuor@geotab.com>
> 
> When an EEPROM is present, tps25750 loads the binary configuration from
> EEPROM. Hence, all we need to do is wait for the device to switch to APP
> mode
> 
> Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> ---
>  drivers/usb/typec/tipd/core.c     | 13 +++++++++++++
>  drivers/usb/typec/tipd/tps6598x.h |  3 +++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index fea139c72d6d..b3d4b2b5bf5f 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -37,6 +37,7 @@
>  #define TPS_REG_STATUS			0x1a
>  #define TPS_REG_SYSTEM_CONF		0x28
>  #define TPS_REG_CTRL_CONF		0x29
> +#define TPS_REG_BOOT_STATUS		0x2D
>  #define TPS_REG_POWER_STATUS		0x3f
>  #define TPS_REG_RX_IDENTITY_SOP		0x48
>  #define TPS_REG_DATA_STATUS		0x5f
> @@ -897,6 +898,17 @@ static int tps25750_apply_patch(struct tps6598x *tps)
>  	int ret;
>  	unsigned long timeout;
>  	u8 mode;
> +	u64 status = 0;
> +
> +	ret = tps6598x_block_read(tps, TPS_REG_BOOT_STATUS, &status, 5);
> +	if (ret)
> +		return ret;
> +	/*
> +	 * Nothing to be done if the configuration
> +	 * is being loaded from EERPOM
> +	 */
> +	if (status & TPS25750_BOOT_STATUS_I2C_EEPROM_PRESENT)
> +		goto wait_for_app;
>  
>  	ret = tps25750_start_patch_burst_mode(tps);
>  	if (ret) {
> @@ -908,6 +920,7 @@ static int tps25750_apply_patch(struct tps6598x *tps)
>  	if (ret)
>  		return ret;
>  
> +wait_for_app:
>  	timeout = jiffies + msecs_to_jiffies(1000);
>  
>  	do {
> diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
> index 527857549d69..5e942c089c27 100644
> --- a/drivers/usb/typec/tipd/tps6598x.h
> +++ b/drivers/usb/typec/tipd/tps6598x.h
> @@ -199,4 +199,7 @@
>  #define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_A    BIT(2)
>  #define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_B    (BIT(2) | BIT(1))
>  
> +/* BOOT STATUS REG*/
> +#define TPS25750_BOOT_STATUS_I2C_EEPROM_PRESENT	BIT(3)

That's not TPS25750 specific bit, so please rename that to
TPS_BOOT_STATUS_I2C_EEPROM_PRESENT

thanks,

-- 
heikki

Re: [PATCH v5 08/15] USB: typec: Add interrupt support for TPS25750

[Heikki Krogerus]

Hi,

On Sun, Sep 17, 2023 at 11:26:32AM -0400, Abdel Alkuor wrote:
> From: Abdel Alkuor <abdelalkuor@geotab.com>
> 
> Update tps6598x interrupt handler to accommodate tps25750 interrupt

You have the "why" explained here, but please also explain what you
are doing - in this case it's not completely clear.

> Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> ---
>  drivers/usb/typec/tipd/core.c | 49 +++++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index bd5436fd88fd..17b3bc480f97 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -120,6 +120,7 @@ struct tps6598x {
>  	enum power_supply_usb_type usb_type;
>  
>  	int wakeup;
> +	u32 status; /* status reg */
>  	u16 pwr_status;
>  	struct delayed_work	wq_poll;
>  	irq_handler_t irq_handler;
> @@ -539,50 +540,71 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
>  	return IRQ_NONE;
>  }
>  
> +static bool tps6598x_has_role_changed(struct tps6598x *tps, u32 status)
> +{
> +	status ^= tps->status;
> +
> +	return status & (TPS_STATUS_PORTROLE | TPS_STATUS_DATAROLE);
> +}
> +
>  static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  {
>  	struct tps6598x *tps = data;
> -	u64 event1 = 0;
> -	u64 event2 = 0;
> +	u64 event[2] = { };
>  	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);
> +	if (tps->is_tps25750) {
> +		ret = tps6598x_block_read(tps, TPS_REG_INT_EVENT1, event, 11);
> +	} else {
> +		ret = tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event[0]);
> +		ret |= tps6598x_read64(tps, TPS_REG_INT_EVENT2, &event[1]);
> +	}
> +
>  	if (ret) {
>  		dev_err(tps->dev, "%s: failed to read events\n", __func__);
>  		goto err_unlock;
>  	}
> -	trace_tps6598x_irq(event1, event2);
> +	trace_tps6598x_irq(event[0], event[1]);
>  
> -	if (!(event1 | event2))
> +	if (!(event[0] | event[1]))
>  		goto err_unlock;
>  
>  	if (!tps6598x_read_status(tps, &status))
>  		goto err_clear_ints;
>  
> -	if ((event1 | event2) & TPS_REG_INT_POWER_STATUS_UPDATE)
> +	if ((event[0] | event[1]) & 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 ((event[0] | event[1]) & 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)
> +	/*
> +	 * data/port roles could be updated independently after
> +	 * 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 ||
> +		tps6598x_has_role_changed(tps, status))

Alignment.

>  		tps6598x_handle_plug_event(tps, status);
>  
> +	tps->status = status;
>  err_clear_ints:
> -	tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event1);
> -	tps6598x_write64(tps, TPS_REG_INT_CLEAR2, event2);
> +	if (tps->is_tps25750) {
> +		tps6598x_block_write(tps, TPS_REG_INT_CLEAR1, event, 11);
> +	} else {
> +		tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event[0]);
> +		tps6598x_write64(tps, TPS_REG_INT_CLEAR2, event[1]);
> +	}
>  
>  err_unlock:
>  	mutex_unlock(&tps->lock);
>  
> -	if (event1 | event2)
> +	if (event[0] | event[1])
>  		return IRQ_HANDLED;
>  	return IRQ_NONE;
>  }
> @@ -1003,7 +1025,6 @@ static int tps6598x_probe(struct i2c_client *client)
>  
>  		irq_handler = cd321x_interrupt;
>  	} else {
> -

You need to fix patch 4 instead - that's where you add that empty
line.

>  		tps->is_tps25750 = of_device_is_compatible(np, "ti,tps25750");
>  		/* Enable power status, data status and plug event interrupts */
>  		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
> -- 
> 2.34.1

-- 
heikki

Re: [PATCH v5 10/15] USB: typec: Add port registration for tps25750

[Heikki Krogerus]

On Sun, Sep 17, 2023 at 11:26:34AM -0400, Abdel Alkuor wrote:
> From: Abdel Alkuor <abdelalkuor@geotab.com>
> 
> TPS25750 doesn't have system configuration register to get dr/pr of the
> current applied binary configuration.
> 
> Get data role from the device node and power role from PD status register.
> 
> Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> ---
>  drivers/usb/typec/tipd/core.c     | 61 ++++++++++++++++++++++++++++++-
>  drivers/usb/typec/tipd/tps6598x.h | 10 +++++
>  2 files changed, 70 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 8218d88a4a06..a97fda68cb54 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -39,6 +39,7 @@
>  #define TPS_REG_CTRL_CONF		0x29
>  #define TPS_REG_BOOT_STATUS		0x2D
>  #define TPS_REG_POWER_STATUS		0x3f
> +#define TPS_REG_PD_STATUS		0x40
>  #define TPS_REG_RX_IDENTITY_SOP		0x48
>  #define TPS_REG_DATA_STATUS		0x5f
>  
> @@ -1028,6 +1029,60 @@ tps6598x_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
>  	return 0;
>  }
>  
> +static int
> +tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
> +{
> +	struct typec_capability typec_cap = { };
> +	const char *data_role;
> +	u8 pd_status;
> +	int ret;
> +
> +	ret = tps6598x_read8(tps, TPS_REG_PD_STATUS, &pd_status);
> +	if (ret)
> +		return ret;
> +
> +	ret = fwnode_property_read_string(fwnode, "data-role", &data_role);
> +	if (ret) {
> +		dev_err(tps->dev, "data-role not found: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = typec_find_port_data_role(data_role);
> +	if (ret < 0) {
> +		dev_err(tps->dev, "unknown data-role: %s\n", data_role);
> +		return ret;
> +	}
> +
> +	typec_cap.data = ret;
> +	typec_cap.revision = USB_TYPEC_REV_1_3;
> +	typec_cap.pd_revision = 0x300;
> +	typec_cap.driver_data = tps;
> +	typec_cap.ops = &tps6598x_ops;
> +	typec_cap.fwnode = fwnode;
> +	typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
> +
> +	switch (TPS_PD_STATUS_PORT_TYPE(pd_status)) {
> +	case TPS_PD_STATUS_PORT_TYPE_SINK_SOURCE:
> +	case TPS_PD_STATUS_PORT_TYPE_SOURCE_SINK:
> +		typec_cap.type = TYPEC_PORT_DRP;
> +		break;
> +	case TPS_PD_STATUS_PORT_TYPE_SINK:
> +		typec_cap.type = TYPEC_PORT_SNK;
> +		break;
> +	case TPS_PD_STATUS_PORT_TYPE_SOURCE:
> +		typec_cap.type = TYPEC_PORT_SRC;
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +
> +	tps->port = typec_register_port(tps->dev, &typec_cap);
> +	if (IS_ERR(tps->port))
> +		return PTR_ERR(tps->port);
> +
> +	return 0;
> +}
> +
>  static int tps6598x_probe(struct i2c_client *client)
>  {
>  	irq_handler_t irq_handler = tps6598x_interrupt;
> @@ -1124,7 +1179,11 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (ret)
>  		goto err_role_put;
>  
> -	ret = tps6598x_register_port(tps, fwnode);
> +	if (np && of_device_is_compatible(np, "ti,tps25750"))

Why aren't you using is_tps25750 here?

> +		ret = tps25750_register_port(tps, fwnode);
> +	else
> +		ret = tps6598x_register_port(tps, fwnode);
> +
>  	if (ret)
>  		goto err_role_put;

thanks,

-- 
heikki

Re: [PATCH v5 15/15] USB: typec: Do not check VID for tps25750

[Heikki Krogerus]

Hi,

On Sun, Sep 17, 2023 at 11:26:39AM -0400, Abdel Alkuor wrote:
> From: Abdel Alkuor <abdelalkuor@geotab.com>
> 
> tps25750 doesn't have VID register, check VID for PD controllers
> other than tps25750
> 
> Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> ---
>  drivers/usb/typec/tipd/core.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 326c23bfa8e6..c1399e12a170 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -1142,10 +1142,6 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (IS_ERR(tps->regmap))
>  		return PTR_ERR(tps->regmap);
>  
> -	ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
> -	if (ret < 0 || !vid)
> -		return -ENODEV;
> -
>  	/*
>  	 * Checking can the adapter handle SMBus protocol. If it can not, the
>  	 * driver needs to take care of block reads separately.
> @@ -1176,6 +1172,12 @@ static int tps6598x_probe(struct i2c_client *client)
>  
>  	tps->irq_handler = irq_handler;
>  
> +	if (!tps->is_tps25750) {
> +		ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
> +		if (ret < 0 || !vid)
> +			return -ENODEV;
> +	}

You need to do this at the same time you enable tps25750, so I'm
guessing in patch 4.

You are also changing the execution order just because of that
is_tps25750. Instead you need to make sure you have that flag set as
soon as possible in the first place, so right after "tps" is
allocated:

        mutex_init(&tps->lock);
        tps->dev = &client->dev;
+       tps->is_tps25750 = of_device_is_compatible(np, "ti,tps25750");
 
        tps->regmap = devm_regmap_init_i2c(client, &tps6598x_regmap_config);
        if (IS_ERR(tps->regmap))


thanks,

-- 
heikki

Re: [PATCH v5 00/15] Add TPS25750 USB type-C PD controller support

[Heikki Krogerus]

On Sun, Sep 17, 2023 at 11:26:24AM -0400, Abdel Alkuor wrote:
> From: Abdel Alkuor <abdelalkuor@geotab.com>
> 
> TPS25750 USB type-C PD controller has the same register offsets as
> tps6598x. The following is a summary of incorporating TPS25750 into
> TPS6598x driver:
> 
> - Only Check VID register (0x00) for TPS6598x and cd321x, as TPS25750 doesn't
>   have VID register.
> 
> - TypeC port registration will be registered differently for each PD
>   controller. TPS6598x uses system configuration register (0x28) to get
>   pr/dr capabilities. On the other hand, TPS25750 will use data role property
>   and PD status register (0x40) to get pr/dr capabilities as TPS25750 doesn't
>   have register 0x28 supported.
> 
> - TPS25750 requires writing a binary configuration to switch PD
>   controller from PTCH mode to APP mode which needs the following changes:
>   - Add PTCH mode to the modes list.
>   - Add an argument to tps6598x_check_mode to return the current mode.
>   - Currently, tps6598x_exec_cmd has cmd timeout hardcoded to 1 second,
>     and doesn't wait before checking DATA_OUT response. In TPS25750, patch 4CCs
>     take longer than 1 second to execute and some requires a delay before
>     checking DATA_OUT. To accommodate that, cmd_timeout and response_delay will
>     be added as arguments to tps6598x_exec_cmd.
>   - Implement applying patch sequence for TPS25750.
> 
> - In pm suspend callback, patch mode needs to be checked and the binary
>   configuration should be applied if needed.
> 
> - For interrupt, TPS25750 has only one event register (0x14) and one mask
>   register (0x16) of 11 bytes each, where TPS6598x has two event
>   and two mask registers of 8 bytes each. Both TPS25750 and TPS65986x
>   shares the same bit field offsets for events/masks/clear but many of
>   there fields are reserved in TPS25750, the following needs to be done in
>   tps6598x_interrupt:
>   - Read EVENT1 register as a block of 11 bytes when tps25750 is present
>   - Write CLEAR1 register as a block of 11 bytes when tps25750 is present
>   - Add trace_tps25750_irq
>   - During testing, I noticed that when a cable is plugged into the PD
>     controller and before PD controller switches to APP mode, there is a
>     lag between dr/pr updates and PlugInsertOrRemoval Event, so a check
>     for dr/pr change needs to be added along TPS_REG_INT_PLUG_EVENT check
> 
> - Add TPS25750 traces for status and power status registers. Trace for
>   data register won't be added as it doesn't exist in the device.
> 
> - Configure sleep mode for TPS25750.

This looks mostly okay, but I'm a bit uncomfortable with flags like
is_tps25750.

I think a better way would be to supply driver data. In it you would
have a callback for everything that needs to be customised.

struct tipd_data {
        int (*interrupt)(int irq, void *data);
        ...
};
...
static const struct tipd_data tps25750_data = {
        .interrupt = tps25750_interrupt,
...

Something like that. You can on top of that still check
device_is_compatible(dev, "...") in some places.


thanks,

-- 
heikki

Re: [PATCH v5 02/15] USB: typec: Add cmd timeout and response delay

[Abdel Alkuor]

On Mon, Sep 18, 2023 at 01:44:48PM +0300, Heikki Krogerus wrote:
> On Sun, Sep 17, 2023 at 11:26:26AM -0400, Abdel Alkuor wrote:
> > Some commands in tps25750 take longer than 1 second
> > to complete, and some responses need some delay before
> > the result becomes available.
> > 
> > Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> > ---
> >  drivers/usb/typec/tipd/core.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > index 37b56ce75f39..a8aee4e1aeba 100644
> > --- a/drivers/usb/typec/tipd/core.c
> > +++ b/drivers/usb/typec/tipd/core.c
> > @@ -284,7 +284,8 @@ static void tps6598x_disconnect(struct tps6598x *tps, u32 status)
> >  
> >  static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
> >  			     size_t in_len, u8 *in_data,
> > -			     size_t out_len, u8 *out_data)
> > +			     size_t out_len, u8 *out_data,
> > +			     u32 cmd_timeout_ms, u32 res_delay_ms)
> 
> It looks like 1s/0s is still the "default", so you could have just
> made this old function a wrapper:
> 
> static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
> 			     size_t in_len, u8 *in_data,
> 			     size_t out_len, u8 *out_data)
> {
>         return tps6598x_exec_cmd_tmo(tps, cmd, in_len, in_data, out_len, out_data, 1000, 0);
> }
Sounds good. I will change it in v6.
> 
> thanks,
> 
> -- 
> heikki

Thanks,
Abdel

Re: [PATCH v5 03/15] USB: typec: Add patch mode to tps6598x

[Abdel Alkuor]

On Mon, Sep 18, 2023 at 02:07:44PM +0300, Heikki Krogerus wrote:
> On Sun, Sep 17, 2023 at 11:26:27AM -0400, Abdel Alkuor wrote:
> > TPS25750 has a patch mode indicating the device requires
> > a configuration to get the device into operational mode
> > 
> > Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> > ---
> >  drivers/usb/typec/tipd/core.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > index a8aee4e1aeba..6d2151325fbb 100644
> > --- a/drivers/usb/typec/tipd/core.c
> > +++ b/drivers/usb/typec/tipd/core.c
> > @@ -68,6 +68,7 @@ enum {
> >  	TPS_MODE_BOOT,
> >  	TPS_MODE_BIST,
> >  	TPS_MODE_DISC,
> > +	TPS_MODE_PTCH,
> >  };
> >  
> >  static const char *const modes[] = {
> > @@ -75,6 +76,7 @@ static const char *const modes[] = {
> >  	[TPS_MODE_BOOT]	= "BOOT",
> >  	[TPS_MODE_BIST]	= "BIST",
> >  	[TPS_MODE_DISC]	= "DISC",
> > +	[TPS_MODE_PTCH] = "PTCH",
> >  };
> >  
> >  /* Unrecognized commands will be replaced with "!CMD" */
> > @@ -576,7 +578,7 @@ static void tps6598x_poll_work(struct work_struct *work)
> >  			   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
> >  }
> >  
> > -static int tps6598x_check_mode(struct tps6598x *tps)
> > +static int tps6598x_check_mode(struct tps6598x *tps, u8 *curr_mode)
> >  {
> >  	char mode[5] = { };
> >  	int ret;
> > @@ -585,8 +587,11 @@ static int tps6598x_check_mode(struct tps6598x *tps)
> >  	if (ret)
> >  		return ret;
> >  
> > -	switch (match_string(modes, ARRAY_SIZE(modes), mode)) {
> > +	*curr_mode = match_string(modes, ARRAY_SIZE(modes), mode);
> > +
> > +	switch (*curr_mode) {
> >  	case TPS_MODE_APP:
> > +	case TPS_MODE_PTCH:
> 
> This check is OK, but it seems that you are not using that curr_mode
> for anything yet, so don't change the paramaters of this function in
> this patch.
> 
> I would also just return the mode. Then this function would be used
> like this:
> 
>         ret = tps6598x_check_mode(...)
>         if (ret < 0)
>                 return ret;
> 
> Now mode = ret.
> 
> >  		return 0;
> >  	case TPS_MODE_BOOT:
> >  		dev_warn(tps->dev, "dead-battery condition\n");
> > @@ -715,6 +720,7 @@ static int tps6598x_probe(struct i2c_client *client)
> >  	u32 vid;
> >  	int ret;
> >  	u64 mask1;
> > +	u8 mode;
> >  
> >  	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
> >  	if (!tps)
> > @@ -759,7 +765,7 @@ static int tps6598x_probe(struct i2c_client *client)
> >  
> >  	tps->irq_handler = irq_handler;
> >  	/* Make sure the controller has application firmware running */
> > -	ret = tps6598x_check_mode(tps);
> > +	ret = tps6598x_check_mode(tps, &mode);
> 
> Actually, if you need to know the mode in here later, then I'm not
> sure tps6592x_check_mode() is useful after that. This is the only
> place it's called. Just read and check the mode in here directly at
> that point.

I will return the mode and check ret directly.
> 
> thanks,
> 
> -- 
> heikki

Thanks,
Abdel

Re: [PATCH v5 04/15] USB: typec: Load TPS25750 patch bundle

[Abdel Alkuor]

On Mon, Sep 18, 2023 at 02:31:54PM +0300, Heikki Krogerus wrote:
> On Sun, Sep 17, 2023 at 11:26:28AM -0400, Abdel Alkuor wrote:
> > From: Abdel Alkuor <abdelalkuor@geotab.com>
> > 
> > TPS25750 controller requires a binary to be loaded with a configuration
> > binary by an EEPROM or a host.
> > 
> > Appling a patch bundling using a host is implemented based on the flow
> > diagram pg.62 in TPS25750 host interface manual.
> > https://www.ti.com/lit/ug/slvuc05a/slvuc05a.pdf
> > 
> > The flow diagram can be summarized as following:
> > - Start the patch loading sequence with patch bundle information by
> >   executing PBMs
> > - Write the whole patch at once
> > - When writing the patch fails, execute PBMe which instructs the PD controller
> >   to end the patching process
> > - After writing the patch successfully, execute PBMc which verifies the patch
> >   integrity and applies the patch internally
> > - Wait for the device to switch into APP mode (normal operation)
> > 
> > The execuation flow diagram polls the events register and then polls the
> > corresponding register related to the event as well before advancing to the next
> > state. Polling the events register is a redundant step, in this implementation
> > only the corresponding register related to the event is polled.
> > 
> > Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> > ---
> >  drivers/usb/typec/tipd/core.c | 237 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 236 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > index 6d2151325fbb..fea139c72d6d 100644
> > --- a/drivers/usb/typec/tipd/core.c
> > +++ b/drivers/usb/typec/tipd/core.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/usb/typec_altmode.h>
> >  #include <linux/usb/role.h>
> >  #include <linux/workqueue.h>
> > +#include <linux/firmware.h>
> >  
> >  #include "tps6598x.h"
> >  #include "trace.h"
> > @@ -43,6 +44,23 @@
> >  /* TPS_REG_SYSTEM_CONF bits */
> >  #define TPS_SYSCONF_PORTINFO(c)		((c) & 7)
> >  
> > +/*
> > + * BPMs task timeout, recommended 5 seconds
> > + * pg.48 TPS2575 Host Interface Technical Reference
> > + * Manual (Rev. A)
> > + * https://www.ti.com/lit/ug/slvuc05a/slvuc05a.pdf
> > + */
> > +#define TPS_BUNDLE_TIMEOUT	0x32
> > +
> > +/* BPMs return code */
> > +#define TPS_TASK_BPMS_INVALID_BUNDLE_SIZE	0x4
> > +#define TPS_TASK_BPMS_INVALID_SLAVE_ADDR	0x5
> > +#define TPS_TASK_BPMS_INVALID_TIMEOUT		0x6
> > +
> > +/* PBMc data out */
> > +#define TPS_PBMC_RC	0 /* Return code */
> > +#define TPS_PBMC_DPCS	2 /* device patch complete status */
> > +
> >  enum {
> >  	TPS_PORTINFO_SINK,
> >  	TPS_PORTINFO_SINK_ACCESSORY,
> > @@ -88,6 +106,8 @@ struct tps6598x {
> >  	struct mutex lock; /* device lock */
> >  	u8 i2c_protocol:1;
> >  
> > +	u8 is_tps25750:1;
> > +
> >  	struct typec_port *port;
> >  	struct typec_partner *partner;
> >  	struct usb_pd_identity partner_identity;
> > @@ -708,6 +728,203 @@ static int devm_tps6598_psy_register(struct tps6598x *tps)
> >  	return PTR_ERR_OR_ZERO(tps->psy);
> >  }
> >  
> > +static int
> > +tps25750_write_firmware(struct tps6598x *tps,
> > +			u8 bpms_addr, const u8 *data, size_t len)
> > +{
> > +	struct i2c_client *client = to_i2c_client(tps->dev);
> > +	int ret;
> > +	u8 slave_addr;
> > +	int timeout;
> > +
> > +	slave_addr = client->addr;
> > +	timeout = client->adapter->timeout;
> > +
> > +	/*
> > +	 * binary configuration size is around ~16Kbytes
> > +	 * which might take some time to finish writing it
> > +	 */
> > +	client->adapter->timeout = msecs_to_jiffies(5000);
> > +	client->addr = bpms_addr;
> > +
> > +	ret = regmap_raw_write(tps->regmap, data[0], &data[1], len - 1);
> > +
> > +	client->addr = slave_addr;
> > +	client->adapter->timeout = timeout;
> > +
> > +	return ret;
> > +}
> > +
> > +static int
> > +tps25750_exec_pbms(struct tps6598x *tps, u8 *in_data, size_t in_len)
> > +{
> > +	int ret;
> > +	u8 rc;
> > +
> > +	ret = tps6598x_exec_cmd(tps, "PBMs", in_len, in_data,
> > +				sizeof(rc), &rc, 4000, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	switch (rc) {
> > +	case TPS_TASK_BPMS_INVALID_BUNDLE_SIZE:
> > +		dev_err(tps->dev, "%s: invalid fw size\n", __func__);
> > +		return -EINVAL;
> > +	case TPS_TASK_BPMS_INVALID_SLAVE_ADDR:
> > +		dev_err(tps->dev, "%s: invalid slave address\n", __func__);
> > +		return -EINVAL;
> > +	case TPS_TASK_BPMS_INVALID_TIMEOUT:
> > +		dev_err(tps->dev, "%s: timed out\n", __func__);
> > +		return -ETIMEDOUT;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int tps25750_abort_patch_process(struct tps6598x *tps)
> > +{
> > +	int ret;
> > +	u8 mode;
> > +
> > +	ret = tps6598x_exec_cmd(tps, "PBMe", 0, NULL, 0, NULL, 1000, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = tps6598x_check_mode(tps, &mode);
> > +	if (mode != TPS_MODE_PTCH)
> > +		dev_err(tps->dev, "failed to switch to \"PTCH\" mode\n");
> > +
> > +	return ret;
> > +}
> > +
> > +static int tps25750_start_patch_burst_mode(struct tps6598x *tps)
> > +{
> > +	int ret;
> > +	const struct firmware *fw;
> > +	const char *firmware_name;
> > +	struct {
> > +		u32 fw_size;
> > +		u8 addr;
> > +		u8 timeout;
> > +	} __packed bpms_data;
> > +
> > +	ret = device_property_read_string(tps->dev, "firmware-name",
> > +					  &firmware_name);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = request_firmware(&fw, firmware_name, tps->dev);
> > +	if (ret) {
> > +		dev_err(tps->dev, "failed to retrieve \"%s\"\n", firmware_name);
> > +		return ret;
> > +	}
> > +
> > +	if (fw->size == 0) {
> > +		ret = -EINVAL;
> > +		goto release_fw;
> > +	}
> > +
> > +	ret = device_property_read_u8(tps->dev, "ti,patch-address", &bpms_data.addr);
> > +	if (ret) {
> > +		dev_err(tps->dev, "failed to get patch address\n");
> > +		return ret;
> > +	}
> > +
> > +	bpms_data.fw_size = fw->size;
> > +	bpms_data.timeout = TPS_BUNDLE_TIMEOUT;
> > +
> > +	ret = tps25750_exec_pbms(tps, (u8 *)&bpms_data, sizeof(bpms_data));
> > +	if (ret)
> > +		goto release_fw;
> > +
> > +	ret = tps25750_write_firmware(tps, bpms_data.addr, fw->data, fw->size);
> > +	if (ret) {
> > +		dev_err(tps->dev, "Failed to write patch %s of %lu bytes\n",
> > +			firmware_name, fw->size);
> > +		goto release_fw;
> > +	}
> > +
> > +	/*
> > +	 * A delay of 500us is required after the firmware is written
> > +	 * based on pg.62 in tps6598x Host Interface Technical
> > +	 * Reference Manual
> > +	 * https://www.ti.com/lit/ug/slvuc05a/slvuc05a.pdf
> > +	 */
> > +	udelay(500);
> > +
> > +release_fw:
> > +	release_firmware(fw);
> > +
> > +	return ret;
> > +}
> > +
> > +static int tps25750_complete_patch_process(struct tps6598x *tps)
> > +{
> > +	int ret;
> > +	u8 out_data[40];
> > +	u8 dummy[2] = { };
> > +
> > +	/*
> > +	 * Without writing something to DATA_IN, this command would
> > +	 * return an error
> > +	 */
> > +	ret = tps6598x_exec_cmd(tps, "PBMc", sizeof(dummy), dummy,
> > +				sizeof(out_data), out_data, 2000, 20);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (out_data[TPS_PBMC_RC]) {
> > +		dev_err(tps->dev,
> > +			"%s: pbmc failed: %u\n", __func__,
> > +			out_data[TPS_PBMC_RC]);
> > +		return -EIO;
> > +	}
> > +
> > +	if (out_data[TPS_PBMC_DPCS]) {
> > +		dev_err(tps->dev,
> > +			"%s: failed device patch complete status: %u\n",
> > +			__func__, out_data[TPS_PBMC_DPCS]);
> > +		return -EIO;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int tps25750_apply_patch(struct tps6598x *tps)
> > +{
> > +	int ret;
> > +	unsigned long timeout;
> > +	u8 mode;
> > +
> > +	ret = tps25750_start_patch_burst_mode(tps);
> > +	if (ret) {
> > +		tps25750_abort_patch_process(tps);
> > +		return ret;
> > +	}
> > +
> > +	ret = tps25750_complete_patch_process(tps);
> > +	if (ret)
> > +		return ret;
> > +
> > +	timeout = jiffies + msecs_to_jiffies(1000);
> > +
> > +	do {
> > +		ret = tps6598x_check_mode(tps, &mode);
> > +		if (ret)
> > +			return ret;
> > +
> > +		if (time_is_before_jiffies(timeout))
> > +			return -ETIMEDOUT;
> > +
> > +	} while (mode != TPS_MODE_APP);
> > +
> > +	dev_info(tps->dev, "controller switched to \"APP\" mode\n");
> > +
> > +	return 0;
> > +};
> > +
> >  static int tps6598x_probe(struct i2c_client *client)
> >  {
> >  	irq_handler_t irq_handler = tps6598x_interrupt;
> > @@ -757,6 +974,8 @@ static int tps6598x_probe(struct i2c_client *client)
> >  
> >  		irq_handler = cd321x_interrupt;
> >  	} else {
> > +
> > +		tps->is_tps25750 = of_device_is_compatible(np, "ti,tps25750");
> >  		/* Enable power status, data status and plug event interrupts */
> >  		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
> >  			TPS_REG_INT_DATA_STATUS_UPDATE |
> > @@ -769,9 +988,15 @@ static int tps6598x_probe(struct i2c_client *client)
> >  	if (ret)
> >  		return ret;
> >  
> > +	if (tps->is_tps25750 && mode == TPS_MODE_PTCH) {
> > +		ret = tps25750_apply_patch(tps);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	ret = tps6598x_write64(tps, TPS_REG_INT_MASK1, mask1);
> >  	if (ret)
> > -		return ret;
> > +		goto err_reset_controller;
> >  
> >  	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
> >  	if (ret < 0)
> > @@ -891,6 +1116,10 @@ static int tps6598x_probe(struct i2c_client *client)
> >  	fwnode_handle_put(fwnode);
> >  err_clear_mask:
> >  	tps6598x_write64(tps, TPS_REG_INT_MASK1, 0);
> > +err_reset_controller:
> > +	/* Reset PD controller to remove any applied patch */
> > +	if (tps->is_tps25750)
> > +		tps6598x_exec_cmd(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
> >  	return ret;
> >  }
> >  
> > @@ -901,9 +1130,14 @@ static void tps6598x_remove(struct i2c_client *client)
> >  	if (!client->irq)
> >  		cancel_delayed_work_sync(&tps->wq_poll);
> >  
> > +	devm_free_irq(tps->dev, client->irq, tps);
> >  	tps6598x_disconnect(tps, 0);
> >  	typec_unregister_port(tps->port);
> >  	usb_role_switch_put(tps->role_sw);
> > +
> > +	/* Reset PD controller to remove any applied patch */
> > +	if (tps->is_tps25750)
> > +		tps6598x_exec_cmd(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
> >  }
> >  
> >  static int __maybe_unused tps6598x_suspend(struct device *dev)
> > @@ -946,6 +1180,7 @@ static const struct dev_pm_ops tps6598x_pm_ops = {
> >  static const struct of_device_id tps6598x_of_match[] = {
> >  	{ .compatible = "ti,tps6598x", },
> >  	{ .compatible = "apple,cd321x", },
> > +	{ .compatible = "ti,tps25750", },
> 
> This is probable OK for now, we can mix this stuff into core.c now, but
> later I want this driver (core.c) to be converted into a library that
> contains only the common functionality. TPS5750 will at that point have
> its own probe/glue driver.
>
Duly noted. 
> >  	{}
> >  };
> >  MODULE_DEVICE_TABLE(of, tps6598x_of_match);
> > -- 
> > 2.34.1
> 
> thanks,
> 
> -- 
> heikki

Abdel

Re: [PATCH v5 05/15] USB: typec: Check for EEPROM present

[Abdel Alkuor]

On Mon, Sep 18, 2023 at 03:45:04PM +0300, Heikki Krogerus wrote:
> On Sun, Sep 17, 2023 at 11:26:29AM -0400, Abdel Alkuor wrote:
> > From: Abdel Alkuor <abdelalkuor@geotab.com>
> > 
> > When an EEPROM is present, tps25750 loads the binary configuration from
> > EEPROM. Hence, all we need to do is wait for the device to switch to APP
> > mode
> > 
> > Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> > ---
> >  drivers/usb/typec/tipd/core.c     | 13 +++++++++++++
> >  drivers/usb/typec/tipd/tps6598x.h |  3 +++
> >  2 files changed, 16 insertions(+)
> > 
> > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > index fea139c72d6d..b3d4b2b5bf5f 100644
> > --- a/drivers/usb/typec/tipd/core.c
> > +++ b/drivers/usb/typec/tipd/core.c
> > @@ -37,6 +37,7 @@
> >  #define TPS_REG_STATUS			0x1a
> >  #define TPS_REG_SYSTEM_CONF		0x28
> >  #define TPS_REG_CTRL_CONF		0x29
> > +#define TPS_REG_BOOT_STATUS		0x2D
> >  #define TPS_REG_POWER_STATUS		0x3f
> >  #define TPS_REG_RX_IDENTITY_SOP		0x48
> >  #define TPS_REG_DATA_STATUS		0x5f
> > @@ -897,6 +898,17 @@ static int tps25750_apply_patch(struct tps6598x *tps)
> >  	int ret;
> >  	unsigned long timeout;
> >  	u8 mode;
> > +	u64 status = 0;
> > +
> > +	ret = tps6598x_block_read(tps, TPS_REG_BOOT_STATUS, &status, 5);
> > +	if (ret)
> > +		return ret;
> > +	/*
> > +	 * Nothing to be done if the configuration
> > +	 * is being loaded from EERPOM
> > +	 */
> > +	if (status & TPS25750_BOOT_STATUS_I2C_EEPROM_PRESENT)
> > +		goto wait_for_app;
> >  
> >  	ret = tps25750_start_patch_burst_mode(tps);
> >  	if (ret) {
> > @@ -908,6 +920,7 @@ static int tps25750_apply_patch(struct tps6598x *tps)
> >  	if (ret)
> >  		return ret;
> >  
> > +wait_for_app:
> >  	timeout = jiffies + msecs_to_jiffies(1000);
> >  
> >  	do {
> > diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
> > index 527857549d69..5e942c089c27 100644
> > --- a/drivers/usb/typec/tipd/tps6598x.h
> > +++ b/drivers/usb/typec/tipd/tps6598x.h
> > @@ -199,4 +199,7 @@
> >  #define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_A    BIT(2)
> >  #define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_B    (BIT(2) | BIT(1))
> >  
> > +/* BOOT STATUS REG*/
> > +#define TPS25750_BOOT_STATUS_I2C_EEPROM_PRESENT	BIT(3)
> 
> That's not TPS25750 specific bit, so please rename that to
> TPS_BOOT_STATUS_I2C_EEPROM_PRESENT
> 
I will fix it in v6.
> thanks,
> 
> -- 
> heikki

Thanks,
Abdel

Re: [PATCH v5 08/15] USB: typec: Add interrupt support for TPS25750

[Abdel Alkuor]

On Mon, Sep 18, 2023 at 03:46:40PM +0300, Heikki Krogerus wrote:
> Hi,
> 
> On Sun, Sep 17, 2023 at 11:26:32AM -0400, Abdel Alkuor wrote:
> > From: Abdel Alkuor <abdelalkuor@geotab.com>
> > 
> > Update tps6598x interrupt handler to accommodate tps25750 interrupt
> 
> You have the "why" explained here, but please also explain what you
> are doing - in this case it's not completely clear.
>
Makes sense. I will add an explanation in v6.
> > Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> > ---
> >  drivers/usb/typec/tipd/core.c | 49 +++++++++++++++++++++++++----------
> >  1 file changed, 35 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > index bd5436fd88fd..17b3bc480f97 100644
> > --- a/drivers/usb/typec/tipd/core.c
> > +++ b/drivers/usb/typec/tipd/core.c
> > @@ -120,6 +120,7 @@ struct tps6598x {
> >  	enum power_supply_usb_type usb_type;
> >  
> >  	int wakeup;
> > +	u32 status; /* status reg */
> >  	u16 pwr_status;
> >  	struct delayed_work	wq_poll;
> >  	irq_handler_t irq_handler;
> > @@ -539,50 +540,71 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
> >  	return IRQ_NONE;
> >  }
> >  
> > +static bool tps6598x_has_role_changed(struct tps6598x *tps, u32 status)
> > +{
> > +	status ^= tps->status;
> > +
> > +	return status & (TPS_STATUS_PORTROLE | TPS_STATUS_DATAROLE);
> > +}
> > +
> >  static irqreturn_t tps6598x_interrupt(int irq, void *data)
> >  {
> >  	struct tps6598x *tps = data;
> > -	u64 event1 = 0;
> > -	u64 event2 = 0;
> > +	u64 event[2] = { };
> >  	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);
> > +	if (tps->is_tps25750) {
> > +		ret = tps6598x_block_read(tps, TPS_REG_INT_EVENT1, event, 11);
> > +	} else {
> > +		ret = tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event[0]);
> > +		ret |= tps6598x_read64(tps, TPS_REG_INT_EVENT2, &event[1]);
> > +	}
> > +
> >  	if (ret) {
> >  		dev_err(tps->dev, "%s: failed to read events\n", __func__);
> >  		goto err_unlock;
> >  	}
> > -	trace_tps6598x_irq(event1, event2);
> > +	trace_tps6598x_irq(event[0], event[1]);
> >  
> > -	if (!(event1 | event2))
> > +	if (!(event[0] | event[1]))
> >  		goto err_unlock;
> >  
> >  	if (!tps6598x_read_status(tps, &status))
> >  		goto err_clear_ints;
> >  
> > -	if ((event1 | event2) & TPS_REG_INT_POWER_STATUS_UPDATE)
> > +	if ((event[0] | event[1]) & 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 ((event[0] | event[1]) & 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)
> > +	/*
> > +	 * data/port roles could be updated independently after
> > +	 * 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 ||
> > +		tps6598x_has_role_changed(tps, status))
> 
> Alignment.
Will be fixed in v6.
> 
> >  		tps6598x_handle_plug_event(tps, status);
> >  
> > +	tps->status = status;
> >  err_clear_ints:
> > -	tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event1);
> > -	tps6598x_write64(tps, TPS_REG_INT_CLEAR2, event2);
> > +	if (tps->is_tps25750) {
> > +		tps6598x_block_write(tps, TPS_REG_INT_CLEAR1, event, 11);
> > +	} else {
> > +		tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event[0]);
> > +		tps6598x_write64(tps, TPS_REG_INT_CLEAR2, event[1]);
> > +	}
> >  
> >  err_unlock:
> >  	mutex_unlock(&tps->lock);
> >  
> > -	if (event1 | event2)
> > +	if (event[0] | event[1])
> >  		return IRQ_HANDLED;
> >  	return IRQ_NONE;
> >  }
> > @@ -1003,7 +1025,6 @@ static int tps6598x_probe(struct i2c_client *client)
> >  
> >  		irq_handler = cd321x_interrupt;
> >  	} else {
> > -
> 
> You need to fix patch 4 instead - that's where you add that empty
> line.
Will be fixed in v6.
> 
> >  		tps->is_tps25750 = of_device_is_compatible(np, "ti,tps25750");
> >  		/* Enable power status, data status and plug event interrupts */
> >  		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
> > -- 
> > 2.34.1
> 
> -- 
> heikki

Thanks,
Abdel

Re: [PATCH v5 10/15] USB: typec: Add port registration for tps25750

[Abdel Alkuor]

On Mon, Sep 18, 2023 at 03:58:15PM +0300, Heikki Krogerus wrote:
> On Sun, Sep 17, 2023 at 11:26:34AM -0400, Abdel Alkuor wrote:
> > From: Abdel Alkuor <abdelalkuor@geotab.com>
> > 
> > TPS25750 doesn't have system configuration register to get dr/pr of the
> > current applied binary configuration.
> > 
> > Get data role from the device node and power role from PD status register.
> > 
> > Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> > ---
> >  drivers/usb/typec/tipd/core.c     | 61 ++++++++++++++++++++++++++++++-
> >  drivers/usb/typec/tipd/tps6598x.h | 10 +++++
> >  2 files changed, 70 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > index 8218d88a4a06..a97fda68cb54 100644
> > --- a/drivers/usb/typec/tipd/core.c
> > +++ b/drivers/usb/typec/tipd/core.c
> > @@ -39,6 +39,7 @@
> >  #define TPS_REG_CTRL_CONF		0x29
> >  #define TPS_REG_BOOT_STATUS		0x2D
> >  #define TPS_REG_POWER_STATUS		0x3f
> > +#define TPS_REG_PD_STATUS		0x40
> >  #define TPS_REG_RX_IDENTITY_SOP		0x48
> >  #define TPS_REG_DATA_STATUS		0x5f
> >  
> > @@ -1028,6 +1029,60 @@ tps6598x_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
> >  	return 0;
> >  }
> >  
> > +static int
> > +tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
> > +{
> > +	struct typec_capability typec_cap = { };
> > +	const char *data_role;
> > +	u8 pd_status;
> > +	int ret;
> > +
> > +	ret = tps6598x_read8(tps, TPS_REG_PD_STATUS, &pd_status);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = fwnode_property_read_string(fwnode, "data-role", &data_role);
> > +	if (ret) {
> > +		dev_err(tps->dev, "data-role not found: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = typec_find_port_data_role(data_role);
> > +	if (ret < 0) {
> > +		dev_err(tps->dev, "unknown data-role: %s\n", data_role);
> > +		return ret;
> > +	}
> > +
> > +	typec_cap.data = ret;
> > +	typec_cap.revision = USB_TYPEC_REV_1_3;
> > +	typec_cap.pd_revision = 0x300;
> > +	typec_cap.driver_data = tps;
> > +	typec_cap.ops = &tps6598x_ops;
> > +	typec_cap.fwnode = fwnode;
> > +	typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
> > +
> > +	switch (TPS_PD_STATUS_PORT_TYPE(pd_status)) {
> > +	case TPS_PD_STATUS_PORT_TYPE_SINK_SOURCE:
> > +	case TPS_PD_STATUS_PORT_TYPE_SOURCE_SINK:
> > +		typec_cap.type = TYPEC_PORT_DRP;
> > +		break;
> > +	case TPS_PD_STATUS_PORT_TYPE_SINK:
> > +		typec_cap.type = TYPEC_PORT_SNK;
> > +		break;
> > +	case TPS_PD_STATUS_PORT_TYPE_SOURCE:
> > +		typec_cap.type = TYPEC_PORT_SRC;
> > +		break;
> > +	default:
> > +		return -ENODEV;
> > +	}
> > +
> > +	tps->port = typec_register_port(tps->dev, &typec_cap);
> > +	if (IS_ERR(tps->port))
> > +		return PTR_ERR(tps->port);
> > +
> > +	return 0;
> > +}
> > +
> >  static int tps6598x_probe(struct i2c_client *client)
> >  {
> >  	irq_handler_t irq_handler = tps6598x_interrupt;
> > @@ -1124,7 +1179,11 @@ static int tps6598x_probe(struct i2c_client *client)
> >  	if (ret)
> >  		goto err_role_put;
> >  
> > -	ret = tps6598x_register_port(tps, fwnode);
> > +	if (np && of_device_is_compatible(np, "ti,tps25750"))
> 
> Why aren't you using is_tps25750 here?
Ops, I missed that. Will be fixed in v6.
> 
> > +		ret = tps25750_register_port(tps, fwnode);
> > +	else
> > +		ret = tps6598x_register_port(tps, fwnode);
> > +
> >  	if (ret)
> >  		goto err_role_put;
> 
> thanks,
> 
> -- 
> heikki

Thanks,
Abdel

Re: [PATCH v5 15/15] USB: typec: Do not check VID for tps25750

[Abdel Alkuor]

On Mon, Sep 18, 2023 at 04:29:43PM +0300, Heikki Krogerus wrote:
> Hi,
> 
> On Sun, Sep 17, 2023 at 11:26:39AM -0400, Abdel Alkuor wrote:
> > From: Abdel Alkuor <abdelalkuor@geotab.com>
> > 
> > tps25750 doesn't have VID register, check VID for PD controllers
> > other than tps25750
> > 
> > Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>
> > ---
> >  drivers/usb/typec/tipd/core.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > index 326c23bfa8e6..c1399e12a170 100644
> > --- a/drivers/usb/typec/tipd/core.c
> > +++ b/drivers/usb/typec/tipd/core.c
> > @@ -1142,10 +1142,6 @@ static int tps6598x_probe(struct i2c_client *client)
> >  	if (IS_ERR(tps->regmap))
> >  		return PTR_ERR(tps->regmap);
> >  
> > -	ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
> > -	if (ret < 0 || !vid)
> > -		return -ENODEV;
> > -
> >  	/*
> >  	 * Checking can the adapter handle SMBus protocol. If it can not, the
> >  	 * driver needs to take care of block reads separately.
> > @@ -1176,6 +1172,12 @@ static int tps6598x_probe(struct i2c_client *client)
> >  
> >  	tps->irq_handler = irq_handler;
> >  
> > +	if (!tps->is_tps25750) {
> > +		ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
> > +		if (ret < 0 || !vid)
> > +			return -ENODEV;
> > +	}
> 
> You need to do this at the same time you enable tps25750, so I'm
> guessing in patch 4.
> 
> You are also changing the execution order just because of that
> is_tps25750. Instead you need to make sure you have that flag set as
> soon as possible in the first place, so right after "tps" is
> allocated:
>
Good point. I will move the check in patch 4 and check it after allocating
tps.
>         mutex_init(&tps->lock);
>         tps->dev = &client->dev;
> +       tps->is_tps25750 = of_device_is_compatible(np, "ti,tps25750");
>  
>         tps->regmap = devm_regmap_init_i2c(client, &tps6598x_regmap_config);
>         if (IS_ERR(tps->regmap))
> 
> 
> thanks,
> 
> -- 
> heikki
Thanks,
Abdel

Re: [PATCH v5 00/15] Add TPS25750 USB type-C PD controller support

[Abdel Alkuor]

On Mon, Sep 18, 2023 at 04:57:49PM +0300, Heikki Krogerus wrote:
> On Sun, Sep 17, 2023 at 11:26:24AM -0400, Abdel Alkuor wrote:
> > From: Abdel Alkuor <abdelalkuor@geotab.com>
> > 
> > TPS25750 USB type-C PD controller has the same register offsets as
> > tps6598x. The following is a summary of incorporating TPS25750 into
> > TPS6598x driver:
> > 
> > - Only Check VID register (0x00) for TPS6598x and cd321x, as TPS25750 doesn't
> >   have VID register.
> > 
> > - TypeC port registration will be registered differently for each PD
> >   controller. TPS6598x uses system configuration register (0x28) to get
> >   pr/dr capabilities. On the other hand, TPS25750 will use data role property
> >   and PD status register (0x40) to get pr/dr capabilities as TPS25750 doesn't
> >   have register 0x28 supported.
> > 
> > - TPS25750 requires writing a binary configuration to switch PD
> >   controller from PTCH mode to APP mode which needs the following changes:
> >   - Add PTCH mode to the modes list.
> >   - Add an argument to tps6598x_check_mode to return the current mode.
> >   - Currently, tps6598x_exec_cmd has cmd timeout hardcoded to 1 second,
> >     and doesn't wait before checking DATA_OUT response. In TPS25750, patch 4CCs
> >     take longer than 1 second to execute and some requires a delay before
> >     checking DATA_OUT. To accommodate that, cmd_timeout and response_delay will
> >     be added as arguments to tps6598x_exec_cmd.
> >   - Implement applying patch sequence for TPS25750.
> > 
> > - In pm suspend callback, patch mode needs to be checked and the binary
> >   configuration should be applied if needed.
> > 
> > - For interrupt, TPS25750 has only one event register (0x14) and one mask
> >   register (0x16) of 11 bytes each, where TPS6598x has two event
> >   and two mask registers of 8 bytes each. Both TPS25750 and TPS65986x
> >   shares the same bit field offsets for events/masks/clear but many of
> >   there fields are reserved in TPS25750, the following needs to be done in
> >   tps6598x_interrupt:
> >   - Read EVENT1 register as a block of 11 bytes when tps25750 is present
> >   - Write CLEAR1 register as a block of 11 bytes when tps25750 is present
> >   - Add trace_tps25750_irq
> >   - During testing, I noticed that when a cable is plugged into the PD
> >     controller and before PD controller switches to APP mode, there is a
> >     lag between dr/pr updates and PlugInsertOrRemoval Event, so a check
> >     for dr/pr change needs to be added along TPS_REG_INT_PLUG_EVENT check
> > 
> > - Add TPS25750 traces for status and power status registers. Trace for
> >   data register won't be added as it doesn't exist in the device.
> > 
> > - Configure sleep mode for TPS25750.
> 
> This looks mostly okay, but I'm a bit uncomfortable with flags like
> is_tps25750.
> 
> I think a better way would be to supply driver data. In it you would
> have a callback for everything that needs to be customised.
> 
> struct tipd_data {
>         int (*interrupt)(int irq, void *data);
>         ...
> };
> ...
> static const struct tipd_data tps25750_data = {
>         .interrupt = tps25750_interrupt,
> ...
> 
> Something like that. You can on top of that still check
> device_is_compatible(dev, "...") in some places.
>
Sounds good. I will create callbacks factory struct as you suggested
and remove the flag.
> 
> thanks,
> 
> -- 
> heikki

Thanks,
Abdel