[PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Javier Carrasco]

This series extends the patch update mechanism to support the tps6598x.

Currently there is only support for the tps25750 part and some
conditional clauses are used to make a special case out of it. Now that
different parts support patch updates, a more general approach is
proposed.

The update mechanism differs from the one required by tps25750 and it is
explained in the commit message as a summary of the application note in
that respect.

Note that the series makes use of the TPS_SETUP_MS introduced in
commit 6a4d4a27f986 ("usb: typec: tps6598x: add reset gpio support"),
which is currently available in usb-next / usb-testing.

A TPS65987D has been used to test this functionality with positive
results.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
Changes in v2:
- Remove probe defeferring mechanism and expect the firmware to be
  available (Heikki Krogerus).
- Link to v1: https://lore.kernel.org/r/20231207-tps6598x_update-v1-0-dc21b5301d91@wolfvision.net

---
Javier Carrasco (4):
      usb: typec: tipd: add init and reset functions to tipd_data
      usb: typec: tipd: add function to request firmware
      usb: typec: tipd: declare in_data in as const in exec_cmd functions
      usb: typec: tipd: add patch update support for tps6598x

 drivers/usb/typec/tipd/core.c     | 150 ++++++++++++++++++++++++++++++++------
 drivers/usb/typec/tipd/tps6598x.h |  18 +++++
 2 files changed, 147 insertions(+), 21 deletions(-)
---
base-commit: 51920207674e9e3475a91d2091583889792df99a
change-id: 20231207-tps6598x_update-632eab69d2ed

Best regards,
-- 
Javier Carrasco <javier.carrasco@wolfvision.net>

[PATCH v2 1/4] usb: typec: tipd: add init and reset functions to tipd_data

[Javier Carrasco]

The current implementation includes a number of special cases for the
tps25750. Nevertheless, init and reset functions can be generalized by
adding function pointers to the tipd_data structure in order to offer
that functionality to other parts without additional conditional
clauses.

Some functionality like the cold reset request (GAID) is shared by the
tps25750 and the tps6598x, so they can use the same reset function.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 drivers/usb/typec/tipd/core.c | 45 +++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 806ef68273ca..f0c4cd571a37 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -115,6 +115,8 @@ struct tipd_data {
 	void (*trace_power_status)(u16 status);
 	void (*trace_status)(u32 status);
 	int (*apply_patch)(struct tps6598x *tps);
+	int (*init)(struct tps6598x *tps);
+	int (*reset)(struct tps6598x *tps);
 };
 
 struct tps6598x {
@@ -1106,6 +1108,11 @@ static int tps25750_apply_patch(struct tps6598x *tps)
 	return 0;
 };
 
+static int cd321x_init(struct tps6598x *tps)
+{
+	return 0;
+}
+
 static int tps25750_init(struct tps6598x *tps)
 {
 	int ret;
@@ -1124,6 +1131,21 @@ static int tps25750_init(struct tps6598x *tps)
 	return 0;
 }
 
+static int tps6598x_init(struct tps6598x *tps)
+{
+	return 0;
+}
+
+static int cd321x_reset(struct tps6598x *tps)
+{
+	return 0;
+}
+
+static int tps6598x_reset(struct tps6598x *tps)
+{
+	return tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
+}
+
 static int
 tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
 {
@@ -1187,7 +1209,6 @@ static int tps6598x_probe(struct i2c_client *client)
 	u32 vid;
 	int ret;
 	u64 mask1;
-	bool is_tps25750;
 
 	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
 	if (!tps)
@@ -1207,8 +1228,7 @@ static int tps6598x_probe(struct i2c_client *client)
 	if (IS_ERR(tps->regmap))
 		return PTR_ERR(tps->regmap);
 
-	is_tps25750 = device_is_compatible(tps->dev, "ti,tps25750");
-	if (!is_tps25750) {
+	if (!device_is_compatible(tps->dev, "ti,tps25750")) {
 		ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
 		if (ret < 0 || !vid)
 			return -ENODEV;
@@ -1251,8 +1271,8 @@ static int tps6598x_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
-	if (is_tps25750 && ret == TPS_MODE_PTCH) {
-		ret = tps25750_init(tps);
+	if (ret == TPS_MODE_PTCH) {
+		ret = tps->data->init(tps);
 		if (ret)
 			return ret;
 	}
@@ -1340,8 +1360,8 @@ static int tps6598x_probe(struct i2c_client *client)
 	tps6598x_write64(tps, TPS_REG_INT_MASK1, 0);
 err_reset_controller:
 	/* Reset PD controller to remove any applied patch */
-	if (is_tps25750)
-		tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
+	tps->data->reset(tps);
+
 	return ret;
 }
 
@@ -1358,8 +1378,7 @@ static void tps6598x_remove(struct i2c_client *client)
 	usb_role_switch_put(tps->role_sw);
 
 	/* Reset PD controller to remove any applied patch */
-	if (device_is_compatible(tps->dev, "ti,tps25750"))
-		tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
+	tps->data->reset(tps);
 
 	if (tps->reset)
 		gpiod_set_value_cansleep(tps->reset, 1);
@@ -1393,7 +1412,7 @@ static int __maybe_unused tps6598x_resume(struct device *dev)
 	if (ret < 0)
 		return ret;
 
-	if (device_is_compatible(tps->dev, "ti,tps25750") && ret == TPS_MODE_PTCH) {
+	if (ret == TPS_MODE_PTCH) {
 		ret = tps25750_init(tps);
 		if (ret)
 			return ret;
@@ -1423,6 +1442,8 @@ static const struct tipd_data cd321x_data = {
 	.register_port = tps6598x_register_port,
 	.trace_power_status = trace_tps6598x_power_status,
 	.trace_status = trace_tps6598x_status,
+	.init = cd321x_init,
+	.reset = cd321x_reset,
 };
 
 static const struct tipd_data tps6598x_data = {
@@ -1430,6 +1451,8 @@ static const struct tipd_data tps6598x_data = {
 	.register_port = tps6598x_register_port,
 	.trace_power_status = trace_tps6598x_power_status,
 	.trace_status = trace_tps6598x_status,
+	.init = tps6598x_init,
+	.reset = tps6598x_reset,
 };
 
 static const struct tipd_data tps25750_data = {
@@ -1438,6 +1461,8 @@ static const struct tipd_data tps25750_data = {
 	.trace_power_status = trace_tps25750_power_status,
 	.trace_status = trace_tps25750_status,
 	.apply_patch = tps25750_apply_patch,
+	.init = tps25750_init,
+	.reset = tps6598x_reset,
 };
 
 static const struct of_device_id tps6598x_of_match[] = {

-- 
2.39.2

Re: [PATCH v2 1/4] usb: typec: tipd: add init and reset functions to tipd_data

[Heikki Krogerus]

On Thu, Dec 14, 2023 at 05:29:09PM +0100, Javier Carrasco wrote:
> The current implementation includes a number of special cases for the
> tps25750. Nevertheless, init and reset functions can be generalized by
> adding function pointers to the tipd_data structure in order to offer
> that functionality to other parts without additional conditional
> clauses.
> 
> Some functionality like the cold reset request (GAID) is shared by the
> tps25750 and the tps6598x, so they can use the same reset function.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 45 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 806ef68273ca..f0c4cd571a37 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -115,6 +115,8 @@ struct tipd_data {
>  	void (*trace_power_status)(u16 status);
>  	void (*trace_status)(u32 status);
>  	int (*apply_patch)(struct tps6598x *tps);
> +	int (*init)(struct tps6598x *tps);
> +	int (*reset)(struct tps6598x *tps);
>  };
>  
>  struct tps6598x {
> @@ -1106,6 +1108,11 @@ static int tps25750_apply_patch(struct tps6598x *tps)
>  	return 0;
>  };
>  
> +static int cd321x_init(struct tps6598x *tps)
> +{
> +	return 0;
> +}
> +
>  static int tps25750_init(struct tps6598x *tps)
>  {
>  	int ret;
> @@ -1124,6 +1131,21 @@ static int tps25750_init(struct tps6598x *tps)
>  	return 0;
>  }
>  
> +static int tps6598x_init(struct tps6598x *tps)
> +{
> +	return 0;
> +}
> +
> +static int cd321x_reset(struct tps6598x *tps)
> +{
> +	return 0;
> +}
> +
> +static int tps6598x_reset(struct tps6598x *tps)
> +{
> +	return tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
> +}
> +
>  static int
>  tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
>  {
> @@ -1187,7 +1209,6 @@ static int tps6598x_probe(struct i2c_client *client)
>  	u32 vid;
>  	int ret;
>  	u64 mask1;
> -	bool is_tps25750;
>  
>  	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
>  	if (!tps)
> @@ -1207,8 +1228,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (IS_ERR(tps->regmap))
>  		return PTR_ERR(tps->regmap);
>  
> -	is_tps25750 = device_is_compatible(tps->dev, "ti,tps25750");
> -	if (!is_tps25750) {
> +	if (!device_is_compatible(tps->dev, "ti,tps25750")) {
>  		ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
>  		if (ret < 0 || !vid)
>  			return -ENODEV;
> @@ -1251,8 +1271,8 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (is_tps25750 && ret == TPS_MODE_PTCH) {
> -		ret = tps25750_init(tps);
> +	if (ret == TPS_MODE_PTCH) {
> +		ret = tps->data->init(tps);
>  		if (ret)
>  			return ret;
>  	}
> @@ -1340,8 +1360,8 @@ static int tps6598x_probe(struct i2c_client *client)
>  	tps6598x_write64(tps, TPS_REG_INT_MASK1, 0);
>  err_reset_controller:
>  	/* Reset PD controller to remove any applied patch */
> -	if (is_tps25750)
> -		tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
> +	tps->data->reset(tps);
> +
>  	return ret;
>  }
>  
> @@ -1358,8 +1378,7 @@ static void tps6598x_remove(struct i2c_client *client)
>  	usb_role_switch_put(tps->role_sw);
>  
>  	/* Reset PD controller to remove any applied patch */
> -	if (device_is_compatible(tps->dev, "ti,tps25750"))
> -		tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
> +	tps->data->reset(tps);
>  
>  	if (tps->reset)
>  		gpiod_set_value_cansleep(tps->reset, 1);
> @@ -1393,7 +1412,7 @@ static int __maybe_unused tps6598x_resume(struct device *dev)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (device_is_compatible(tps->dev, "ti,tps25750") && ret == TPS_MODE_PTCH) {
> +	if (ret == TPS_MODE_PTCH) {
>  		ret = tps25750_init(tps);
>  		if (ret)
>  			return ret;
> @@ -1423,6 +1442,8 @@ static const struct tipd_data cd321x_data = {
>  	.register_port = tps6598x_register_port,
>  	.trace_power_status = trace_tps6598x_power_status,
>  	.trace_status = trace_tps6598x_status,
> +	.init = cd321x_init,
> +	.reset = cd321x_reset,
>  };
>  
>  static const struct tipd_data tps6598x_data = {
> @@ -1430,6 +1451,8 @@ static const struct tipd_data tps6598x_data = {
>  	.register_port = tps6598x_register_port,
>  	.trace_power_status = trace_tps6598x_power_status,
>  	.trace_status = trace_tps6598x_status,
> +	.init = tps6598x_init,
> +	.reset = tps6598x_reset,
>  };
>  
>  static const struct tipd_data tps25750_data = {
> @@ -1438,6 +1461,8 @@ static const struct tipd_data tps25750_data = {
>  	.trace_power_status = trace_tps25750_power_status,
>  	.trace_status = trace_tps25750_status,
>  	.apply_patch = tps25750_apply_patch,
> +	.init = tps25750_init,
> +	.reset = tps6598x_reset,
>  };
>  
>  static const struct of_device_id tps6598x_of_match[] = {
> 
> -- 
> 2.39.2

-- 
heikki

Re: [PATCH v2 1/4] usb: typec: tipd: add init and reset functions to tipd_data

[Roger Quadros]

On 14/12/2023 18:29, Javier Carrasco wrote:
> The current implementation includes a number of special cases for the
> tps25750. Nevertheless, init and reset functions can be generalized by
> adding function pointers to the tipd_data structure in order to offer
> that functionality to other parts without additional conditional
> clauses.
> 
> Some functionality like the cold reset request (GAID) is shared by the
> tps25750 and the tps6598x, so they can use the same reset function.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  drivers/usb/typec/tipd/core.c | 45 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 806ef68273ca..f0c4cd571a37 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -115,6 +115,8 @@ struct tipd_data {
>  	void (*trace_power_status)(u16 status);
>  	void (*trace_status)(u32 status);
>  	int (*apply_patch)(struct tps6598x *tps);
> +	int (*init)(struct tps6598x *tps);
> +	int (*reset)(struct tps6598x *tps);
>  };
>  
>  struct tps6598x {
> @@ -1106,6 +1108,11 @@ static int tps25750_apply_patch(struct tps6598x *tps)
>  	return 0;
>  };
>  
> +static int cd321x_init(struct tps6598x *tps)
> +{
> +	return 0;
> +}
> +
>  static int tps25750_init(struct tps6598x *tps)
>  {
>  	int ret;
> @@ -1124,6 +1131,21 @@ static int tps25750_init(struct tps6598x *tps)
>  	return 0;
>  }
>  
> +static int tps6598x_init(struct tps6598x *tps)
> +{
> +	return 0;
> +}
> +
> +static int cd321x_reset(struct tps6598x *tps)
> +{
> +	return 0;
> +}
> +
> +static int tps6598x_reset(struct tps6598x *tps)
> +{
> +	return tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
> +}
> +
>  static int
>  tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
>  {
> @@ -1187,7 +1209,6 @@ static int tps6598x_probe(struct i2c_client *client)
>  	u32 vid;
>  	int ret;
>  	u64 mask1;
> -	bool is_tps25750;
>  
>  	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
>  	if (!tps)
> @@ -1207,8 +1228,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (IS_ERR(tps->regmap))
>  		return PTR_ERR(tps->regmap);
>  
> -	is_tps25750 = device_is_compatible(tps->dev, "ti,tps25750");
> -	if (!is_tps25750) {
> +	if (!device_is_compatible(tps->dev, "ti,tps25750")) {
>  		ret = tps6598x_read32(tps, TPS_REG_VID, &vid);
>  		if (ret < 0 || !vid)
>  			return -ENODEV;
> @@ -1251,8 +1271,8 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (is_tps25750 && ret == TPS_MODE_PTCH) {
> -		ret = tps25750_init(tps);
> +	if (ret == TPS_MODE_PTCH) {
> +		ret = tps->data->init(tps);
>  		if (ret)
>  			return ret;
>  	}
> @@ -1340,8 +1360,8 @@ static int tps6598x_probe(struct i2c_client *client)
>  	tps6598x_write64(tps, TPS_REG_INT_MASK1, 0);
>  err_reset_controller:
>  	/* Reset PD controller to remove any applied patch */
> -	if (is_tps25750)
> -		tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
> +	tps->data->reset(tps);
> +
>  	return ret;
>  }
>  
> @@ -1358,8 +1378,7 @@ static void tps6598x_remove(struct i2c_client *client)
>  	usb_role_switch_put(tps->role_sw);
>  
>  	/* Reset PD controller to remove any applied patch */
> -	if (device_is_compatible(tps->dev, "ti,tps25750"))
> -		tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
> +	tps->data->reset(tps);
>  
>  	if (tps->reset)
>  		gpiod_set_value_cansleep(tps->reset, 1);
> @@ -1393,7 +1412,7 @@ static int __maybe_unused tps6598x_resume(struct device *dev)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (device_is_compatible(tps->dev, "ti,tps25750") && ret == TPS_MODE_PTCH) {
> +	if (ret == TPS_MODE_PTCH) {

Won't this function will be invoked for all variants?
If so, why are we calling a tsp25750 specific function here?

>  		ret = tps25750_init(tps);
>  		if (ret)
>  			return ret;
> @@ -1423,6 +1442,8 @@ static const struct tipd_data cd321x_data = {
>  	.register_port = tps6598x_register_port,
>  	.trace_power_status = trace_tps6598x_power_status,
>  	.trace_status = trace_tps6598x_status,
> +	.init = cd321x_init,
> +	.reset = cd321x_reset,
>  };
>  
>  static const struct tipd_data tps6598x_data = {
> @@ -1430,6 +1451,8 @@ static const struct tipd_data tps6598x_data = {
>  	.register_port = tps6598x_register_port,
>  	.trace_power_status = trace_tps6598x_power_status,
>  	.trace_status = trace_tps6598x_status,
> +	.init = tps6598x_init,
> +	.reset = tps6598x_reset,
>  };
>  
>  static const struct tipd_data tps25750_data = {
> @@ -1438,6 +1461,8 @@ static const struct tipd_data tps25750_data = {
>  	.trace_power_status = trace_tps25750_power_status,
>  	.trace_status = trace_tps25750_status,
>  	.apply_patch = tps25750_apply_patch,
> +	.init = tps25750_init,
> +	.reset = tps6598x_reset,
>  };
>  
>  static const struct of_device_id tps6598x_of_match[] = {
> 

-- 
cheers,
-roger

Re: [PATCH v2 1/4] usb: typec: tipd: add init and reset functions to tipd_data

[Javier Carrasco]

On 04.01.24 17:14, Roger Quadros wrote:
>> @@ -1393,7 +1412,7 @@ static int __maybe_unused tps6598x_resume(struct device *dev)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	if (device_is_compatible(tps->dev, "ti,tps25750") && ret == TPS_MODE_PTCH) {
>> +	if (ret == TPS_MODE_PTCH) {
> 
> Won't this function will be invoked for all variants?
> If so, why are we calling a tsp25750 specific function here?
> 
>>  		ret = tps25750_init(tps);
>>  		if (ret)
>>  			return ret;

Hi Roger,

good catch. The device-specific init function should be called instead,
as it is already done in the probe function:

  	ret =  tps->data->init(tps);
  	if (ret)
  		return ret;

I will send the fix asap.

Thanks a lot for your feedback and best regards,
Javier Carrasco

[PATCH v2 2/4] usb: typec: tipd: add function to request firmware

[Javier Carrasco]

The firmware request process is device agnostic and can be used for
other parts.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 drivers/usb/typec/tipd/core.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index f0c4cd571a37..83e5eeecdf5c 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -873,6 +873,30 @@ tps6598x_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
 	return 0;
 }
 
+static int tps_request_firmware(struct tps6598x *tps, const struct firmware **fw)
+{
+	const char *firmware_name;
+	int ret;
+
+	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) {
+		release_firmware(*fw);
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
 static int
 tps25750_write_firmware(struct tps6598x *tps,
 			u8 bpms_addr, const u8 *data, size_t len)
@@ -961,16 +985,9 @@ static int tps25750_start_patch_burst_mode(struct tps6598x *tps)
 	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);
+	ret = tps_request_firmware(tps, &fw);
+	if (ret)
 		return ret;
-	}
-
-	if (fw->size == 0) {
-		ret = -EINVAL;
-		goto release_fw;
-	}
 
 	ret = of_property_match_string(np, "reg-names", "patch-address");
 	if (ret < 0) {

-- 
2.39.2

Re: [PATCH v2 2/4] usb: typec: tipd: add function to request firmware

[Heikki Krogerus]

On Thu, Dec 14, 2023 at 05:29:10PM +0100, Javier Carrasco wrote:
> The firmware request process is device agnostic and can be used for
> other parts.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index f0c4cd571a37..83e5eeecdf5c 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -873,6 +873,30 @@ tps6598x_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
>  	return 0;
>  }
>  
> +static int tps_request_firmware(struct tps6598x *tps, const struct firmware **fw)
> +{
> +	const char *firmware_name;
> +	int ret;
> +
> +	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) {
> +		release_firmware(*fw);
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>  static int
>  tps25750_write_firmware(struct tps6598x *tps,
>  			u8 bpms_addr, const u8 *data, size_t len)
> @@ -961,16 +985,9 @@ static int tps25750_start_patch_burst_mode(struct tps6598x *tps)
>  	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);
> +	ret = tps_request_firmware(tps, &fw);
> +	if (ret)
>  		return ret;
> -	}
> -
> -	if (fw->size == 0) {
> -		ret = -EINVAL;
> -		goto release_fw;
> -	}
>  
>  	ret = of_property_match_string(np, "reg-names", "patch-address");
>  	if (ret < 0) {
> 
> -- 
> 2.39.2

-- 
heikki

[PATCH v2 3/4] usb: typec: tipd: declare in_data in as const in exec_cmd functions

[Javier Carrasco]

The input data passed to execute commands with tps6598x_exec_cmd()
is not supposed to be modified by the function. Moreover, this data is
passed to tps6598x_exec_cmd_tmo() and finally to tps6598x_block_write(),
which expects a const pointer.

The current implementation does not produce any bugs, but it discards
const qualifiers from the pointers passed as arguments. This leads to
compile issues if 'discarded-qualifiers' is active and a const pointer
is passed to the function, which is the case if data from a firmware
structure is passed to execute update commands. Adding the const
modifier to in_data prevents such issues and provides code consistency.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 drivers/usb/typec/tipd/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 83e5eeecdf5c..7f4bbc0629b0 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -330,7 +330,7 @@ static void tps6598x_disconnect(struct tps6598x *tps, u32 status)
 }
 
 static int tps6598x_exec_cmd_tmo(struct tps6598x *tps, const char *cmd,
-			     size_t in_len, u8 *in_data,
+			     size_t in_len, const u8 *in_data,
 			     size_t out_len, u8 *out_data,
 			     u32 cmd_timeout_ms, u32 res_delay_ms)
 {
@@ -396,7 +396,7 @@ static int tps6598x_exec_cmd_tmo(struct tps6598x *tps, const char *cmd,
 }
 
 static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
-			     size_t in_len, u8 *in_data,
+			     size_t in_len, const u8 *in_data,
 			     size_t out_len, u8 *out_data)
 {
 	return tps6598x_exec_cmd_tmo(tps, cmd, in_len, in_data,

-- 
2.39.2

Re: [PATCH v2 3/4] usb: typec: tipd: declare in_data in as const in exec_cmd functions

[Heikki Krogerus]

On Thu, Dec 14, 2023 at 05:29:11PM +0100, Javier Carrasco wrote:
> The input data passed to execute commands with tps6598x_exec_cmd()
> is not supposed to be modified by the function. Moreover, this data is
> passed to tps6598x_exec_cmd_tmo() and finally to tps6598x_block_write(),
> which expects a const pointer.
> 
> The current implementation does not produce any bugs, but it discards
> const qualifiers from the pointers passed as arguments. This leads to
> compile issues if 'discarded-qualifiers' is active and a const pointer
> is passed to the function, which is the case if data from a firmware
> structure is passed to execute update commands. Adding the const
> modifier to in_data prevents such issues and provides code consistency.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 83e5eeecdf5c..7f4bbc0629b0 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -330,7 +330,7 @@ static void tps6598x_disconnect(struct tps6598x *tps, u32 status)
>  }
>  
>  static int tps6598x_exec_cmd_tmo(struct tps6598x *tps, const char *cmd,
> -			     size_t in_len, u8 *in_data,
> +			     size_t in_len, const u8 *in_data,
>  			     size_t out_len, u8 *out_data,
>  			     u32 cmd_timeout_ms, u32 res_delay_ms)
>  {
> @@ -396,7 +396,7 @@ static int tps6598x_exec_cmd_tmo(struct tps6598x *tps, const char *cmd,
>  }
>  
>  static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
> -			     size_t in_len, u8 *in_data,
> +			     size_t in_len, const u8 *in_data,
>  			     size_t out_len, u8 *out_data)
>  {
>  	return tps6598x_exec_cmd_tmo(tps, cmd, in_len, in_data,
> 
> -- 
> 2.39.2

-- 
heikki

[PATCH v2 4/4] usb: typec: tipd: add patch update support for tps6598x

[Javier Carrasco]

The TPS6598x PD controller supports firmware updates that can be loaded
either from an external flash memory or a host using the device's I2C
host interface. This patch implements the second approach, which is
especially relevant if no flash memory is available.

In order to make patch bundle updates, a series of tasks (special
commands) must be sent to the device as it is documented in the
TPS65987DDH and TPS65988DH Host Interface Technical Reference Manual[1],
section 4.11 (Patch Bundle Update Tasks).

The update sequence is as follows:
1. PTCs - Start Patch Load Sequence: the proposed approach includes
   device and application configuration data.
2. PTCd - Patch Download: 64-byte data chunks must be sent until the end
   of the firmware file is reached (the last chunk may be shorter).
3. PTCc - Patch Data Transfer Complete: ends the patch loading sequence.

After this sequence and if no errors occurred, the device will change
its mode to 'APP' after SETUP_MS milliseconds, and then it will be ready
for normal operation.

[1] https://www.ti.com/lit/ug/slvubh2b/slvubh2b.pdf?ts=1697623299919&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FTPS65987D

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 drivers/usb/typec/tipd/core.c     | 68 ++++++++++++++++++++++++++++++++++++++-
 drivers/usb/typec/tipd/tps6598x.h | 18 +++++++++++
 2 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 7f4bbc0629b0..a956eb976906 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -1125,6 +1125,71 @@ static int tps25750_apply_patch(struct tps6598x *tps)
 	return 0;
 };
 
+static int tps6598x_apply_patch(struct tps6598x *tps)
+{
+	u8 in = TPS_PTCS_CONTENT_DEV | TPS_PTCS_CONTENT_APP;
+	u8 out[TPS_MAX_LEN] = {0};
+	size_t in_len = sizeof(in);
+	size_t copied_bytes = 0;
+	size_t bytes_left;
+	const struct firmware *fw;
+	const char *firmware_name;
+	int ret;
+
+	ret = device_property_read_string(tps->dev, "firmware-name",
+					  &firmware_name);
+	if (ret)
+		return ret;
+
+	ret = tps_request_firmware(tps, &fw);
+	if (ret)
+		return ret;
+
+	ret = tps6598x_exec_cmd(tps, "PTCs", in_len, &in,
+				TPS_PTCS_OUT_BYTES, out);
+	if (ret || out[TPS_PTCS_STATUS] == TPS_PTCS_STATUS_FAIL) {
+		if (!ret)
+			ret = -EBUSY;
+		dev_err(tps->dev, "Update start failed (%d)\n", ret);
+		goto release_fw;
+	}
+
+	bytes_left = fw->size;
+	while (bytes_left) {
+		if (bytes_left < TPS_MAX_LEN)
+			in_len = bytes_left;
+		else
+			in_len = TPS_MAX_LEN;
+		ret = tps6598x_exec_cmd(tps, "PTCd", in_len,
+					fw->data + copied_bytes,
+					TPS_PTCD_OUT_BYTES, out);
+		if (ret || out[TPS_PTCD_TRANSFER_STATUS] ||
+		    out[TPS_PTCD_LOADING_STATE] == TPS_PTCD_LOAD_ERR) {
+			if (!ret)
+				ret = -EBUSY;
+			dev_err(tps->dev, "Patch download failed (%d)\n", ret);
+			goto release_fw;
+		}
+		copied_bytes += in_len;
+		bytes_left -= in_len;
+	}
+
+	ret = tps6598x_exec_cmd(tps, "PTCc", 0, NULL, TPS_PTCC_OUT_BYTES, out);
+	if (ret || out[TPS_PTCC_DEV] || out[TPS_PTCC_APP]) {
+		if (!ret)
+			ret = -EBUSY;
+		dev_err(tps->dev, "Update completion failed (%d)\n", ret);
+		goto release_fw;
+	}
+	msleep(TPS_SETUP_MS);
+	dev_info(tps->dev, "Firmware update succeeded\n");
+
+release_fw:
+	release_firmware(fw);
+
+	return ret;
+};
+
 static int cd321x_init(struct tps6598x *tps)
 {
 	return 0;
@@ -1150,7 +1215,7 @@ static int tps25750_init(struct tps6598x *tps)
 
 static int tps6598x_init(struct tps6598x *tps)
 {
-	return 0;
+	return tps->data->apply_patch(tps);
 }
 
 static int cd321x_reset(struct tps6598x *tps)
@@ -1468,6 +1533,7 @@ static const struct tipd_data tps6598x_data = {
 	.register_port = tps6598x_register_port,
 	.trace_power_status = trace_tps6598x_power_status,
 	.trace_status = trace_tps6598x_status,
+	.apply_patch = tps6598x_apply_patch,
 	.init = tps6598x_init,
 	.reset = tps6598x_reset,
 };
diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
index 01609bf509e4..89b24519463a 100644
--- a/drivers/usb/typec/tipd/tps6598x.h
+++ b/drivers/usb/typec/tipd/tps6598x.h
@@ -235,4 +235,22 @@
 /* SLEEP CONF REG */
 #define TPS_SLEEP_CONF_SLEEP_MODE_ALLOWED	BIT(0)
 
+/* Start Patch Download Sequence */
+#define TPS_PTCS_CONTENT_APP			BIT(0)
+#define TPS_PTCS_CONTENT_DEV			BIT(1)
+#define TPS_PTCS_OUT_BYTES			4
+#define TPS_PTCS_STATUS				1
+
+#define TPS_PTCS_STATUS_FAIL			0x80
+/* Patch Download */
+#define TPS_PTCD_OUT_BYTES			10
+#define TPS_PTCD_TRANSFER_STATUS		1
+#define TPS_PTCD_LOADING_STATE			2
+
+#define TPS_PTCD_LOAD_ERR			0x09
+/* Patch Download Complete */
+#define TPS_PTCC_OUT_BYTES			4
+#define TPS_PTCC_DEV				2
+#define TPS_PTCC_APP				3
+
 #endif /* __TPS6598X_H__ */

-- 
2.39.2

Re: [PATCH v2 4/4] usb: typec: tipd: add patch update support for tps6598x

[Heikki Krogerus]

On Thu, Dec 14, 2023 at 05:29:12PM +0100, Javier Carrasco wrote:
> The TPS6598x PD controller supports firmware updates that can be loaded
> either from an external flash memory or a host using the device's I2C
> host interface. This patch implements the second approach, which is
> especially relevant if no flash memory is available.
> 
> In order to make patch bundle updates, a series of tasks (special
> commands) must be sent to the device as it is documented in the
> TPS65987DDH and TPS65988DH Host Interface Technical Reference Manual[1],
> section 4.11 (Patch Bundle Update Tasks).
> 
> The update sequence is as follows:
> 1. PTCs - Start Patch Load Sequence: the proposed approach includes
>    device and application configuration data.
> 2. PTCd - Patch Download: 64-byte data chunks must be sent until the end
>    of the firmware file is reached (the last chunk may be shorter).
> 3. PTCc - Patch Data Transfer Complete: ends the patch loading sequence.
> 
> After this sequence and if no errors occurred, the device will change
> its mode to 'APP' after SETUP_MS milliseconds, and then it will be ready
> for normal operation.
> 
> [1] https://www.ti.com/lit/ug/slvubh2b/slvubh2b.pdf?ts=1697623299919&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FTPS65987D
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c     | 68 ++++++++++++++++++++++++++++++++++++++-
>  drivers/usb/typec/tipd/tps6598x.h | 18 +++++++++++
>  2 files changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 7f4bbc0629b0..a956eb976906 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -1125,6 +1125,71 @@ static int tps25750_apply_patch(struct tps6598x *tps)
>  	return 0;
>  };
>  
> +static int tps6598x_apply_patch(struct tps6598x *tps)
> +{
> +	u8 in = TPS_PTCS_CONTENT_DEV | TPS_PTCS_CONTENT_APP;
> +	u8 out[TPS_MAX_LEN] = {0};
> +	size_t in_len = sizeof(in);
> +	size_t copied_bytes = 0;
> +	size_t bytes_left;
> +	const struct firmware *fw;
> +	const char *firmware_name;
> +	int ret;
> +
> +	ret = device_property_read_string(tps->dev, "firmware-name",
> +					  &firmware_name);
> +	if (ret)
> +		return ret;
> +
> +	ret = tps_request_firmware(tps, &fw);
> +	if (ret)
> +		return ret;
> +
> +	ret = tps6598x_exec_cmd(tps, "PTCs", in_len, &in,
> +				TPS_PTCS_OUT_BYTES, out);
> +	if (ret || out[TPS_PTCS_STATUS] == TPS_PTCS_STATUS_FAIL) {
> +		if (!ret)
> +			ret = -EBUSY;
> +		dev_err(tps->dev, "Update start failed (%d)\n", ret);
> +		goto release_fw;
> +	}
> +
> +	bytes_left = fw->size;
> +	while (bytes_left) {
> +		if (bytes_left < TPS_MAX_LEN)
> +			in_len = bytes_left;
> +		else
> +			in_len = TPS_MAX_LEN;
> +		ret = tps6598x_exec_cmd(tps, "PTCd", in_len,
> +					fw->data + copied_bytes,
> +					TPS_PTCD_OUT_BYTES, out);
> +		if (ret || out[TPS_PTCD_TRANSFER_STATUS] ||
> +		    out[TPS_PTCD_LOADING_STATE] == TPS_PTCD_LOAD_ERR) {
> +			if (!ret)
> +				ret = -EBUSY;
> +			dev_err(tps->dev, "Patch download failed (%d)\n", ret);
> +			goto release_fw;
> +		}
> +		copied_bytes += in_len;
> +		bytes_left -= in_len;
> +	}
> +
> +	ret = tps6598x_exec_cmd(tps, "PTCc", 0, NULL, TPS_PTCC_OUT_BYTES, out);
> +	if (ret || out[TPS_PTCC_DEV] || out[TPS_PTCC_APP]) {
> +		if (!ret)
> +			ret = -EBUSY;
> +		dev_err(tps->dev, "Update completion failed (%d)\n", ret);
> +		goto release_fw;
> +	}
> +	msleep(TPS_SETUP_MS);
> +	dev_info(tps->dev, "Firmware update succeeded\n");
> +
> +release_fw:
> +	release_firmware(fw);
> +
> +	return ret;
> +};
> +
>  static int cd321x_init(struct tps6598x *tps)
>  {
>  	return 0;
> @@ -1150,7 +1215,7 @@ static int tps25750_init(struct tps6598x *tps)
>  
>  static int tps6598x_init(struct tps6598x *tps)
>  {
> -	return 0;
> +	return tps->data->apply_patch(tps);
>  }
>  
>  static int cd321x_reset(struct tps6598x *tps)
> @@ -1468,6 +1533,7 @@ static const struct tipd_data tps6598x_data = {
>  	.register_port = tps6598x_register_port,
>  	.trace_power_status = trace_tps6598x_power_status,
>  	.trace_status = trace_tps6598x_status,
> +	.apply_patch = tps6598x_apply_patch,
>  	.init = tps6598x_init,
>  	.reset = tps6598x_reset,
>  };
> diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
> index 01609bf509e4..89b24519463a 100644
> --- a/drivers/usb/typec/tipd/tps6598x.h
> +++ b/drivers/usb/typec/tipd/tps6598x.h
> @@ -235,4 +235,22 @@
>  /* SLEEP CONF REG */
>  #define TPS_SLEEP_CONF_SLEEP_MODE_ALLOWED	BIT(0)
>  
> +/* Start Patch Download Sequence */
> +#define TPS_PTCS_CONTENT_APP			BIT(0)
> +#define TPS_PTCS_CONTENT_DEV			BIT(1)
> +#define TPS_PTCS_OUT_BYTES			4
> +#define TPS_PTCS_STATUS				1
> +
> +#define TPS_PTCS_STATUS_FAIL			0x80
> +/* Patch Download */
> +#define TPS_PTCD_OUT_BYTES			10
> +#define TPS_PTCD_TRANSFER_STATUS		1
> +#define TPS_PTCD_LOADING_STATE			2
> +
> +#define TPS_PTCD_LOAD_ERR			0x09
> +/* Patch Download Complete */
> +#define TPS_PTCC_OUT_BYTES			4
> +#define TPS_PTCC_DEV				2
> +#define TPS_PTCC_APP				3
> +
>  #endif /* __TPS6598X_H__ */
> 
> -- 
> 2.39.2

-- 
heikki

Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Jai Luthra]

[-- Attachment #1: Type: text/plain, Size: 3322 bytes --]

Hi Javier, Greg,

On Dec 14, 2023 at 17:29:08 +0100, Javier Carrasco wrote:
> This series extends the patch update mechanism to support the tps6598x.
> 
> Currently there is only support for the tps25750 part and some
> conditional clauses are used to make a special case out of it. Now that
> different parts support patch updates, a more general approach is
> proposed.
> 
> The update mechanism differs from the one required by tps25750 and it is
> explained in the commit message as a summary of the application note in
> that respect.
> 
> Note that the series makes use of the TPS_SETUP_MS introduced in
> commit 6a4d4a27f986 ("usb: typec: tps6598x: add reset gpio support"),
> which is currently available in usb-next / usb-testing.
> 
> A TPS65987D has been used to test this functionality with positive
> results.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
> Changes in v2:
> - Remove probe defeferring mechanism and expect the firmware to be
>   available (Heikki Krogerus).
> - Link to v1: 
> https://lore.kernel.org/r/20231207-tps6598x_update-v1-0-dc21b5301d91@wolfvision.net
> 

FYI, this series breaks boot for TI SK-AM62A and SK-AM62 which use 
TPS6598x as the USB-C PD chip.

The platforms stopped booting since next-20240103 [1], and reverting 
this series [4] seems to fix the issue [2]

Is there any change needed in the board device-tree [3] and bindings?

We don't specify any firmware in the device-tree node, but seems like 
that is an assumption in this series. I tried reverting it (below 
change) but that did *not* help with the stuck boot.

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index a956eb976906..fa3bd7349265 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -1139,7 +1139,7 @@ static int tps6598x_apply_patch(struct tps6598x *tps)
        ret = device_property_read_string(tps->dev, "firmware-name",
                                          &firmware_name);
        if (ret)
-               return ret;
+               return 0;

        ret = tps_request_firmware(tps, &fw);
        if (ret)


[1] https://linux.kernelci.org/soc/ti/job/next/kernel/next-20240103/plan/baseline-nfs/
[2] https://gist.github.com/jailuthra/0c077176585e4df2f8b78f7784087865
[3] https://gitlab.com/linux-kernel/linux-next/-/blob/master/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts#L305
[4] https://github.com/jailuthra/linux/commits/next-20240103-tps6598-fix/

> ---
> Javier Carrasco (4):
>       usb: typec: tipd: add init and reset functions to tipd_data
>       usb: typec: tipd: add function to request firmware
>       usb: typec: tipd: declare in_data in as const in exec_cmd functions
>       usb: typec: tipd: add patch update support for tps6598x
> 
>  drivers/usb/typec/tipd/core.c     | 150 ++++++++++++++++++++++++++++++++------
>  drivers/usb/typec/tipd/tps6598x.h |  18 +++++
>  2 files changed, 147 insertions(+), 21 deletions(-)
> ---
> base-commit: 51920207674e9e3475a91d2091583889792df99a
> change-id: 20231207-tps6598x_update-632eab69d2ed
> 
> Best regards,
> -- 
> Javier Carrasco <javier.carrasco@wolfvision.net>
> 
> 


-- 
Thanks,
Jai

GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Jai Luthra]

[-- Attachment #1: Type: text/plain, Size: 4041 bytes --]

Hi Javier,

On Jan 04, 2024 at 19:50:05 +0530, Jai Luthra wrote:
> Hi Javier, Greg,
> 
> On Dec 14, 2023 at 17:29:08 +0100, Javier Carrasco wrote:
> > This series extends the patch update mechanism to support the tps6598x.
> > 
> > Currently there is only support for the tps25750 part and some
> > conditional clauses are used to make a special case out of it. Now that
> > different parts support patch updates, a more general approach is
> > proposed.
> > 
> > The update mechanism differs from the one required by tps25750 and it is
> > explained in the commit message as a summary of the application note in
> > that respect.
> > 
> > Note that the series makes use of the TPS_SETUP_MS introduced in
> > commit 6a4d4a27f986 ("usb: typec: tps6598x: add reset gpio support"),
> > which is currently available in usb-next / usb-testing.
> > 
> > A TPS65987D has been used to test this functionality with positive
> > results.
> > 
> > Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> > ---
> > Changes in v2:
> > - Remove probe defeferring mechanism and expect the firmware to be
> >   available (Heikki Krogerus).
> > - Link to v1: 
> > https://lore.kernel.org/r/20231207-tps6598x_update-v1-0-dc21b5301d91@wolfvision.net
> > 
> 
> FYI, this series breaks boot for TI SK-AM62A and SK-AM62 which use 
> TPS6598x as the USB-C PD chip.
> 
> The platforms stopped booting since next-20240103 [1], and reverting 
> this series [4] seems to fix the issue [2]
> 
> Is there any change needed in the board device-tree [3] and bindings?
> 
> We don't specify any firmware in the device-tree node, but seems like 
> that is an assumption in this series. I tried reverting it (below 
> change) but that did *not* help with the stuck boot.
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index a956eb976906..fa3bd7349265 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -1139,7 +1139,7 @@ static int tps6598x_apply_patch(struct tps6598x *tps)
>         ret = device_property_read_string(tps->dev, "firmware-name",
>                                           &firmware_name);
>         if (ret)
> -               return ret;
> +               return 0;
> 
>         ret = tps_request_firmware(tps, &fw);
>         if (ret)
> 
> 
> [1] https://linux.kernelci.org/soc/ti/job/next/kernel/next-20240103/plan/baseline-nfs/
> [2] https://gist.github.com/jailuthra/0c077176585e4df2f8b78f7784087865
> [3] https://gitlab.com/linux-kernel/linux-next/-/blob/master/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts#L305
> [4] https://github.com/jailuthra/linux/commits/next-20240103-tps6598-fix/

The following change seems to fix boot on SK-AM62A without reverting 
this whole series:

------------------

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index a956eb976906a5..8ba2aa05db519b 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -1223,11 +1223,16 @@ static int cd321x_reset(struct tps6598x *tps)
 	return 0;
 }
 
-static int tps6598x_reset(struct tps6598x *tps)
+static int tps25750_reset(struct tps6598x *tps)
 {
 	return tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
 }
 
+static int tps6598x_reset(struct tps6598x *tps)
+{
+	return 0;
+}
+
 static int
 tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
 {
@@ -1545,7 +1550,7 @@ static const struct tipd_data tps25750_data = {
 	.trace_status = trace_tps25750_status,
 	.apply_patch = tps25750_apply_patch,
 	.init = tps25750_init,
-	.reset = tps6598x_reset,
+	.reset = tps25750_reset,
 };
 
 static const struct of_device_id tps6598x_of_match[] = {

------------------

I am not an expert on this, will let you/others decide on what should be 
the correct way to reset TPS6598x for patching without breaking this SK.


-- 
Thanks,
Jai

GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Roger Quadros]

On 04/01/2024 17:47, Jai Luthra wrote:
> Hi Javier,
> 
> On Jan 04, 2024 at 19:50:05 +0530, Jai Luthra wrote:
>> Hi Javier, Greg,
>>
>> On Dec 14, 2023 at 17:29:08 +0100, Javier Carrasco wrote:
>>> This series extends the patch update mechanism to support the tps6598x.
>>>
>>> Currently there is only support for the tps25750 part and some
>>> conditional clauses are used to make a special case out of it. Now that
>>> different parts support patch updates, a more general approach is
>>> proposed.
>>>
>>> The update mechanism differs from the one required by tps25750 and it is
>>> explained in the commit message as a summary of the application note in
>>> that respect.
>>>
>>> Note that the series makes use of the TPS_SETUP_MS introduced in
>>> commit 6a4d4a27f986 ("usb: typec: tps6598x: add reset gpio support"),
>>> which is currently available in usb-next / usb-testing.
>>>
>>> A TPS65987D has been used to test this functionality with positive
>>> results.
>>>
>>> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
>>> ---
>>> Changes in v2:
>>> - Remove probe defeferring mechanism and expect the firmware to be
>>>   available (Heikki Krogerus).
>>> - Link to v1: 
>>> https://lore.kernel.org/r/20231207-tps6598x_update-v1-0-dc21b5301d91@wolfvision.net
>>>
>>
>> FYI, this series breaks boot for TI SK-AM62A and SK-AM62 which use 
>> TPS6598x as the USB-C PD chip.
>>
>> The platforms stopped booting since next-20240103 [1], and reverting 
>> this series [4] seems to fix the issue [2]
>>
>> Is there any change needed in the board device-tree [3] and bindings?
>>
>> We don't specify any firmware in the device-tree node, but seems like 
>> that is an assumption in this series. I tried reverting it (below 
>> change) but that did *not* help with the stuck boot.
>>
>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>> index a956eb976906..fa3bd7349265 100644
>> --- a/drivers/usb/typec/tipd/core.c
>> +++ b/drivers/usb/typec/tipd/core.c
>> @@ -1139,7 +1139,7 @@ static int tps6598x_apply_patch(struct tps6598x *tps)
>>         ret = device_property_read_string(tps->dev, "firmware-name",
>>                                           &firmware_name);
>>         if (ret)
>> -               return ret;
>> +               return 0;
>>
>>         ret = tps_request_firmware(tps, &fw);
>>         if (ret)
>>
>>
>> [1] https://linux.kernelci.org/soc/ti/job/next/kernel/next-20240103/plan/baseline-nfs/
>> [2] https://gist.github.com/jailuthra/0c077176585e4df2f8b78f7784087865
>> [3] https://gitlab.com/linux-kernel/linux-next/-/blob/master/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts#L305
>> [4] https://github.com/jailuthra/linux/commits/next-20240103-tps6598-fix/
> 
> The following change seems to fix boot on SK-AM62A without reverting 
> this whole series:
> 
> ------------------
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index a956eb976906a5..8ba2aa05db519b 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -1223,11 +1223,16 @@ static int cd321x_reset(struct tps6598x *tps)
>  	return 0;
>  }
>  
> -static int tps6598x_reset(struct tps6598x *tps)
> +static int tps25750_reset(struct tps6598x *tps)
>  {
>  	return tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
>  }
>  
> +static int tps6598x_reset(struct tps6598x *tps)
> +{
> +	return 0;
> +}
> +
>  static int
>  tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
>  {
> @@ -1545,7 +1550,7 @@ static const struct tipd_data tps25750_data = {
>  	.trace_status = trace_tps25750_status,
>  	.apply_patch = tps25750_apply_patch,
>  	.init = tps25750_init,
> -	.reset = tps6598x_reset,
> +	.reset = tps25750_reset,
>  };
>  
>  static const struct of_device_id tps6598x_of_match[] = {
> 
> ------------------
> 
> I am not an expert on this, will let you/others decide on what should be 
> the correct way to reset TPS6598x for patching without breaking this SK.
> 
> 

This looks like a correct fix to me.
Could you please send a proper PATCH with Fixes tag? Thanks!

-- 
cheers,
-roger

Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Javier Carrasco]

On 04.01.24 17:15, Roger Quadros wrote:
> 
> 
> On 04/01/2024 17:47, Jai Luthra wrote:
>> Hi Javier,
>> The following change seems to fix boot on SK-AM62A without reverting 
>> this whole series:
>>
>> ------------------
>>
>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>> index a956eb976906a5..8ba2aa05db519b 100644
>> --- a/drivers/usb/typec/tipd/core.c
>> +++ b/drivers/usb/typec/tipd/core.c
>> @@ -1223,11 +1223,16 @@ static int cd321x_reset(struct tps6598x *tps)
>>  	return 0;
>>  }
>>  
>> -static int tps6598x_reset(struct tps6598x *tps)
>> +static int tps25750_reset(struct tps6598x *tps)
>>  {
>>  	return tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
>>  }
>>  
>> +static int tps6598x_reset(struct tps6598x *tps)
>> +{
>> +	return 0;
>> +}
>> +
>>  static int
>>  tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
>>  {
>> @@ -1545,7 +1550,7 @@ static const struct tipd_data tps25750_data = {
>>  	.trace_status = trace_tps25750_status,
>>  	.apply_patch = tps25750_apply_patch,
>>  	.init = tps25750_init,
>> -	.reset = tps6598x_reset,
>> +	.reset = tps25750_reset,
>>  };
>>  
>>  static const struct of_device_id tps6598x_of_match[] = {
>>
>> ------------------
>>
>> I am not an expert on this, will let you/others decide on what should be 
>> the correct way to reset TPS6598x for patching without breaking this SK.
>>
>>
> 
> This looks like a correct fix to me.
> Could you please send a proper PATCH with Fixes tag? Thanks!
> 
Hi Roger,

that fix only removes the reset function and does nothing instead, but
the reset call is identical for both devices (hence why there was a
single function for both devices). As I mentioned in my reply to Jai
Luthra, I would like to know why the reset is triggered and why that
should not happen.

Thanks and best regards,
Javier Carrasco

Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Roger Quadros]

On 04/01/2024 18:36, Javier Carrasco wrote:
> On 04.01.24 17:15, Roger Quadros wrote:
>>
>>
>> On 04/01/2024 17:47, Jai Luthra wrote:
>>> Hi Javier,
>>> The following change seems to fix boot on SK-AM62A without reverting 
>>> this whole series:
>>>
>>> ------------------
>>>
>>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>>> index a956eb976906a5..8ba2aa05db519b 100644
>>> --- a/drivers/usb/typec/tipd/core.c
>>> +++ b/drivers/usb/typec/tipd/core.c
>>> @@ -1223,11 +1223,16 @@ static int cd321x_reset(struct tps6598x *tps)
>>>  	return 0;
>>>  }
>>>  
>>> -static int tps6598x_reset(struct tps6598x *tps)
>>> +static int tps25750_reset(struct tps6598x *tps)
>>>  {
>>>  	return tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
>>>  }
>>>  
>>> +static int tps6598x_reset(struct tps6598x *tps)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>>  static int
>>>  tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
>>>  {
>>> @@ -1545,7 +1550,7 @@ static const struct tipd_data tps25750_data = {
>>>  	.trace_status = trace_tps25750_status,
>>>  	.apply_patch = tps25750_apply_patch,
>>>  	.init = tps25750_init,
>>> -	.reset = tps6598x_reset,
>>> +	.reset = tps25750_reset,
>>>  };
>>>  
>>>  static const struct of_device_id tps6598x_of_match[] = {
>>>
>>> ------------------
>>>
>>> I am not an expert on this, will let you/others decide on what should be 
>>> the correct way to reset TPS6598x for patching without breaking this SK.
>>>
>>>
>>
>> This looks like a correct fix to me.
>> Could you please send a proper PATCH with Fixes tag? Thanks!
>>
> Hi Roger,
> 
> that fix only removes the reset function and does nothing instead, but
> the reset call is identical for both devices (hence why there was a
> single function for both devices). As I mentioned in my reply to Jai

This is incorrect.

Look at the original code, the GAID command is issued only if it is a
tps25750 device.

Below is your part of the code that replaces it with reset() ops.

> @@ -1340,8 +1360,8 @@ static int tps6598x_probe(struct i2c_client *client)
>  	tps6598x_write64(tps, TPS_REG_INT_MASK1, 0);
>  err_reset_controller:
>  	/* Reset PD controller to remove any applied patch */
> -	if (is_tps25750)
> -		tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
> +	tps->data->reset(tps);
> +
>  	return ret;
>  }

which means the GAID command will be executed for both tps25750 and tps6598x
as you have a single reset function for both.
This is a problem for boards using the tps6598x.

-- 
cheers,
-roger

Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Javier Carrasco]

On 04.01.24 18:08, Roger Quadros wrote:
> 
> 
> On 04/01/2024 18:36, Javier Carrasco wrote:
>> Hi Roger,
>>
>> that fix only removes the reset function and does nothing instead, but
>> the reset call is identical for both devices (hence why there was a
>> single function for both devices). As I mentioned in my reply to Jai
> 
> This is incorrect.
> 
> Look at the original code, the GAID command is issued only if it is a
> tps25750 device.
> 
> Below is your part of the code that replaces it with reset() ops.
> 
>> @@ -1340,8 +1360,8 @@ static int tps6598x_probe(struct i2c_client *client)
>>  	tps6598x_write64(tps, TPS_REG_INT_MASK1, 0);
>>  err_reset_controller:
>>  	/* Reset PD controller to remove any applied patch */
>> -	if (is_tps25750)
>> -		tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
>> +	tps->data->reset(tps);
>> +
>>  	return ret;
>>  }
> 
> which means the GAID command will be executed for both tps25750 and tps6598x
> as you have a single reset function for both.
> This is a problem for boards using the tps6598x.
> 
I have to admit that the GAID for the tps6598x should have been added
separately, and not right away with the function pointers. I added extra
functionality that was not expected. On the other hand, the GAID command
is supported by the tps6598x as well (Technical Reference Manual, page
113). Hence why I was surprised that using the same command for the
tps6598x is a problem.

Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Dhruva Gole]

Hi all,

On Jan 04, 2024 at 18:15:36 +0200, Roger Quadros wrote:
> 
> 
> On 04/01/2024 17:47, Jai Luthra wrote:
> > Hi Javier,
> > 
> > On Jan 04, 2024 at 19:50:05 +0530, Jai Luthra wrote:
> >> Hi Javier, Greg,
> >>
> >> On Dec 14, 2023 at 17:29:08 +0100, Javier Carrasco wrote:
> >>> This series extends the patch update mechanism to support the tps6598x.
> >>>
> >>> Currently there is only support for the tps25750 part and some
> >>> conditional clauses are used to make a special case out of it. Now that
> >>> different parts support patch updates, a more general approach is
> >>> proposed.
> >>>
> >>> The update mechanism differs from the one required by tps25750 and it is
> >>> explained in the commit message as a summary of the application note in
> >>> that respect.
> >>>
> >>> Note that the series makes use of the TPS_SETUP_MS introduced in
> >>> commit 6a4d4a27f986 ("usb: typec: tps6598x: add reset gpio support"),
> >>> which is currently available in usb-next / usb-testing.
> >>>
> >>> A TPS65987D has been used to test this functionality with positive
> >>> results.
> >>>
> >>> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> >>> ---
> >>> Changes in v2:
> >>> - Remove probe defeferring mechanism and expect the firmware to be
> >>>   available (Heikki Krogerus).
> >>> - Link to v1: 
> >>> https://lore.kernel.org/r/20231207-tps6598x_update-v1-0-dc21b5301d91@wolfvision.net
> >>>
> >>
> >> FYI, this series breaks boot for TI SK-AM62A and SK-AM62 which use 
> >> TPS6598x as the USB-C PD chip.

This series also broke boot on TI SK-AM62x [0].

> >>
> >> The platforms stopped booting since next-20240103 [1], and reverting 
> >> this series [4] seems to fix the issue [2]
> >>
> >> Is there any change needed in the board device-tree [3] and bindings?
> >>
> >> We don't specify any firmware in the device-tree node, but seems like 
> >> that is an assumption in this series. I tried reverting it (below 
> >> change) but that did *not* help with the stuck boot.
> >>
> >> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> >> index a956eb976906..fa3bd7349265 100644
> >> --- a/drivers/usb/typec/tipd/core.c
> >> +++ b/drivers/usb/typec/tipd/core.c
> >> @@ -1139,7 +1139,7 @@ static int tps6598x_apply_patch(struct tps6598x *tps)
> >>         ret = device_property_read_string(tps->dev, "firmware-name",
> >>                                           &firmware_name);
> >>         if (ret)
> >> -               return ret;
> >> +               return 0;
> >>
> >>         ret = tps_request_firmware(tps, &fw);
> >>         if (ret)
> >>
> >>
> >> [1] https://linux.kernelci.org/soc/ti/job/next/kernel/next-20240103/plan/baseline-nfs/
> >> [2] https://gist.github.com/jailuthra/0c077176585e4df2f8b78f7784087865
> >> [3] https://gitlab.com/linux-kernel/linux-next/-/blob/master/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts#L305
> >> [4] https://github.com/jailuthra/linux/commits/next-20240103-tps6598-fix/
> > 
> > The following change seems to fix boot on SK-AM62A without reverting 
> > this whole series:
> > 
> > ------------------
> > 
> > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > index a956eb976906a5..8ba2aa05db519b 100644
> > --- a/drivers/usb/typec/tipd/core.c
> > +++ b/drivers/usb/typec/tipd/core.c
> > @@ -1223,11 +1223,16 @@ static int cd321x_reset(struct tps6598x *tps)
> >  	return 0;
> >  }
> >  
> > -static int tps6598x_reset(struct tps6598x *tps)
> > +static int tps25750_reset(struct tps6598x *tps)
> >  {
> >  	return tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
> >  }
> >  
> > +static int tps6598x_reset(struct tps6598x *tps)
> > +{
> > +	return 0;
> > +}
> > +
> >  static int
> >  tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
> >  {
> > @@ -1545,7 +1550,7 @@ static const struct tipd_data tps25750_data = {
> >  	.trace_status = trace_tps25750_status,
> >  	.apply_patch = tps25750_apply_patch,
> >  	.init = tps25750_init,
> > -	.reset = tps6598x_reset,
> > +	.reset = tps25750_reset,
> >  };
> >  
> >  static const struct of_device_id tps6598x_of_match[] = {
> > 
> > ------------------
> > 
> > I am not an expert on this, will let you/others decide on what should be 
> > the correct way to reset TPS6598x for patching without breaking this SK.
> > 
> > 
> 
> This looks like a correct fix to me.
> Could you please send a proper PATCH with Fixes tag? Thanks!

Thanks for reviewing this Roger, the same patch above worked for me to
fix SK-AM62x as well [1].

[0] https://storage.kernelci.org/next/master/next-20240103/arm64/defconfig/gcc-10/lab-ti/baseline-nfs-am62xx_sk-fs.txt
[1] https://gist.github.com/DhruvaG2000/326b5d7fab4be95f20cd0aac4125f577

-- 
Best regards,
Dhruva Gole <d-gole@ti.com>

Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Javier Carrasco]

On 04.01.24 19:52, Dhruva Gole wrote:

> 
> This series also broke boot on TI SK-AM62x [0].
> 

>>
>> This looks like a correct fix to me.
>> Could you please send a proper PATCH with Fixes tag? Thanks!
> 
> Thanks for reviewing this Roger, the same patch above worked for me to
> fix SK-AM62x as well [1].
> 
> [0] https://storage.kernelci.org/next/master/next-20240103/arm64/defconfig/gcc-10/lab-ti/baseline-nfs-am62xx_sk-fs.txt
> [1] https://gist.github.com/DhruvaG2000/326b5d7fab4be95f20cd0aac4125f577
> 
Hi Dhruva,

I am glad that you guys found a fix that quickly.

it seems that you guys work for the device manufacturer (because of your
email addresses), so I was wondering if you could explain (or provide
the documentation) why the tps6598x should not receive the GAID command
and a reset crashes the system. Everything looks exactly the same as for
the tps25750, but in that case there are no complaints from sending a
cold reset.

Thanks again and best regards,
Javier Carrasco

Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Roger Quadros]

On 04/01/2024 22:15, Javier Carrasco wrote:
> On 04.01.24 19:52, Dhruva Gole wrote:
> 
>>
>> This series also broke boot on TI SK-AM62x [0].
>>
> 
>>>
>>> This looks like a correct fix to me.
>>> Could you please send a proper PATCH with Fixes tag? Thanks!
>>
>> Thanks for reviewing this Roger, the same patch above worked for me to
>> fix SK-AM62x as well [1].
>>
>> [0] https://storage.kernelci.org/next/master/next-20240103/arm64/defconfig/gcc-10/lab-ti/baseline-nfs-am62xx_sk-fs.txt
>> [1] https://gist.github.com/DhruvaG2000/326b5d7fab4be95f20cd0aac4125f577
>>
> Hi Dhruva,
> 
> I am glad that you guys found a fix that quickly.
> 
> it seems that you guys work for the device manufacturer (because of your
> email addresses), so I was wondering if you could explain (or provide
> the documentation) why the tps6598x should not receive the GAID command
> and a reset crashes the system. Everything looks exactly the same as for
> the tps25750, but in that case there are no complaints from sending a
> cold reset.

Looking at the kernel logs I don't see any crashes.
Looks like the baseline-nfs.login test failed due to some reason [2].
I cannot see why though.

[1] https://linux.kernelci.org/soc/ti/job/next/kernel/next-20240103/plan/baseline-nfs/
[2] https://linux.kernelci.org/test/plan/id/659561d445debed180c795fc/

-- 
cheers,
-roger

Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Javier Carrasco]

Hi Jai Luthra,
On 04.01.24 16:47, Jai Luthra wrote>> FYI, this series breaks boot for
TI SK-AM62A and SK-AM62 which use
>> TPS6598x as the USB-C PD chip.
>>
>> The platforms stopped booting since next-20240103 [1], and reverting 
>> this series [4] seems to fix the issue [2]
>>
>> Is there any change needed in the board device-tree [3] and bindings?
>>
>> We don't specify any firmware in the device-tree node, but seems like 
>> that is an assumption in this series. I tried reverting it (below 
>> change) but that did *not* help with the stuck boot.
>>Thanks a lot for your high-quality feedback. I am glad to see that you
even found a solution to the issue.

The firmware update only happens if the device is in patch mode ('PTCH'
in the Mode register - 0x03), which is the expected behavior because the
device waits in that mode until a patch arrives. That is probably the
reason why your first attempt did not work (no update was triggered).

The problem seems to be related to the reset function, as you already
noticed. That function is only called in suspend/resume, if the probe
fails and in the remove function.

Did the probe function fail and if so, could you see why? The reset
function is identical for the tps25750 and tps6598x ('GAID' with no
parameters), so I wonder why that should be the source of the problem.
> The following change seems to fix boot on SK-AM62A without reverting 
> this whole series:
> 
> ------------------
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index a956eb976906a5..8ba2aa05db519b 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -1223,11 +1223,16 @@ static int cd321x_reset(struct tps6598x *tps)
>  	return 0;
>  }
>  
> -static int tps6598x_reset(struct tps6598x *tps)
> +static int tps25750_reset(struct tps6598x *tps)
>  {
>  	return tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
>  }
>  
> +static int tps6598x_reset(struct tps6598x *tps)
> +{
> +	return 0;
> +}
> +
>  static int
>  tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
>  {
> @@ -1545,7 +1550,7 @@ static const struct tipd_data tps25750_data = {
>  	.trace_status = trace_tps25750_status,
>  	.apply_patch = tps25750_apply_patch,
>  	.init = tps25750_init,
> -	.reset = tps6598x_reset,
> +	.reset = tps25750_reset,
>  };
>  
>  static const struct of_device_id tps6598x_of_match[] = {
> 
> ------------------
> 
> I am not an expert on this, will let you/others decide on what should be 
> the correct way to reset TPS6598x for patching without breaking this SK.
> 
> 

The driver did not support cold resets before, but that does not mean
that they should not happen like it does for the tps25750. Your fix just
removes the reset function for the tps6598x, and I would like to know
why the reset happened in the fist place.

Thanks and best regards,
Javier Carrasco

Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Jai Luthra]

[-- Attachment #1: Type: text/plain, Size: 4142 bytes --]

On Jan 04, 2024 at 17:25:27 +0100, Javier Carrasco wrote:
> Hi Jai Luthra,
> On 04.01.24 16:47, Jai Luthra wrote>> FYI, this series breaks boot for
> TI SK-AM62A and SK-AM62 which use
> >> TPS6598x as the USB-C PD chip.
> >>
> >> The platforms stopped booting since next-20240103 [1], and reverting 
> >> this series [4] seems to fix the issue [2]
> >>
> >> Is there any change needed in the board device-tree [3] and bindings?
> >>
> >> We don't specify any firmware in the device-tree node, but seems like 
> >> that is an assumption in this series. I tried reverting it (below 
> >> change) but that did *not* help with the stuck boot.
> >>Thanks a lot for your high-quality feedback. I am glad to see that you
> even found a solution to the issue.
> 
> The firmware update only happens if the device is in patch mode ('PTCH'
> in the Mode register - 0x03), which is the expected behavior because the
> device waits in that mode until a patch arrives. That is probably the
> reason why your first attempt did not work (no update was triggered).

Understood. Btw your mail client seems to mess up quotes/spacing above.

> 
> The problem seems to be related to the reset function, as you already
> noticed. That function is only called in suspend/resume, if the probe
> fails and in the remove function.
> 
> Did the probe function fail and if so, could you see why? The reset
> function is identical for the tps25750 and tps6598x ('GAID' with no
> parameters), so I wonder why that should be the source of the problem.

I added some prints and can see that the probe fails once at 
fwnode_usb_role_switch_get() because the other endpoint (of USB device) 
is not yet probed. It then re-probes later where it passes.

The GAID reset being done unconditionally in your series seems to cause 
the board to get stuck in the boot process when it hits the above error 
due to probe-order between USB subsystem and this IC. My guess would be 
SoC stops getting power because we reset the PD chip?

Anyway, I will send below change as a separate "Fixes:" patch for now, 
to keep how things as they were before your series.

If you have a better architecture in mind that can reset only when PTCH 
has been applied and not for other probe defers, feel free to send it on 
top of it.

> > The following change seems to fix boot on SK-AM62A without reverting 
> > this whole series:
> > 
> > ------------------
> > 
> > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > index a956eb976906a5..8ba2aa05db519b 100644
> > --- a/drivers/usb/typec/tipd/core.c
> > +++ b/drivers/usb/typec/tipd/core.c
> > @@ -1223,11 +1223,16 @@ static int cd321x_reset(struct tps6598x *tps)
> >  	return 0;
> >  }
> >  
> > -static int tps6598x_reset(struct tps6598x *tps)
> > +static int tps25750_reset(struct tps6598x *tps)
> >  {
> >  	return tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
> >  }
> >  
> > +static int tps6598x_reset(struct tps6598x *tps)
> > +{
> > +	return 0;
> > +}
> > +
> >  static int
> >  tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
> >  {
> > @@ -1545,7 +1550,7 @@ static const struct tipd_data tps25750_data = {
> >  	.trace_status = trace_tps25750_status,
> >  	.apply_patch = tps25750_apply_patch,
> >  	.init = tps25750_init,
> > -	.reset = tps6598x_reset,
> > +	.reset = tps25750_reset,
> >  };
> >  
> >  static const struct of_device_id tps6598x_of_match[] = {
> > 
> > ------------------
> > 
> > I am not an expert on this, will let you/others decide on what should be 
> > the correct way to reset TPS6598x for patching without breaking this SK.
> > 
> > 
> 
> The driver did not support cold resets before, but that does not mean
> that they should not happen like it does for the tps25750. Your fix just
> removes the reset function for the tps6598x, and I would like to know
> why the reset happened in the fist place.
> 
> Thanks and best regards,
> Javier Carrasco

-- 
Thanks,
Jai

GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Javier Carrasco]

On 05.01.24 09:12, Jai Luthra wrote:
> I added some prints and can see that the probe fails once at 
> fwnode_usb_role_switch_get() because the other endpoint (of USB device) 
> is not yet probed. It then re-probes later where it passes.
> 
> The GAID reset being done unconditionally in your series seems to cause 
> the board to get stuck in the boot process when it hits the above error 
> due to probe-order between USB subsystem and this IC. My guess would be 
> SoC stops getting power because we reset the PD chip?
> 
> Anyway, I will send below change as a separate "Fixes:" patch for now, 
> to keep how things as they were before your series.
> 

My biggest concern is that we are sending GAID for the tps25750 under
the same circumstances. Could we not have the same problem with that
device? We would be resetting the PD controller and the SoC would stop
getting power as well, right? Or is there anything device-specific that
would avoid that?

> If you have a better architecture in mind that can reset only when PTCH 
> has been applied and not for other probe defers, feel free to send it on 
> top of it.
> 

I added the cold reset to have the same behavior upon probe failures for
both devices, given that they use the same command. But if that can
cause problems, let's leave the reset alone...

Best regards,
Javier Carrasco

Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Jai Luthra]

[-- Attachment #1: Type: text/plain, Size: 1898 bytes --]

Hi Javier, Abdel,

On Jan 05, 2024 at 10:49:22 +0100, Javier Carrasco wrote:
> On 05.01.24 09:12, Jai Luthra wrote:
> > I added some prints and can see that the probe fails once at 
> > fwnode_usb_role_switch_get() because the other endpoint (of USB device) 
> > is not yet probed. It then re-probes later where it passes.
> > 
> > The GAID reset being done unconditionally in your series seems to cause 
> > the board to get stuck in the boot process when it hits the above error 
> > due to probe-order between USB subsystem and this IC. My guess would be 
> > SoC stops getting power because we reset the PD chip?
> > 
> > Anyway, I will send below change as a separate "Fixes:" patch for now, 
> > to keep how things as they were before your series.
> > 
> 
> My biggest concern is that we are sending GAID for the tps25750 under
> the same circumstances. Could we not have the same problem with that
> device? We would be resetting the PD controller and the SoC would stop
> getting power as well, right? Or is there anything device-specific that
> would avoid that?
> 

Yes I would guess same problem can happen depending on probe order of 
the remote-endpoint node, but I don't see any upstream platform using 
ti,tps25750 compatible, so I have no way to confirm.

Maybe Abdel can comment on how it works, as he added the GAID reset for 
tps25750.

> > If you have a better architecture in mind that can reset only when PTCH 
> > has been applied and not for other probe defers, feel free to send it on 
> > top of it.
> > 
> 
> I added the cold reset to have the same behavior upon probe failures for
> both devices, given that they use the same command. But if that can
> cause problems, let's leave the reset alone...
> 
> Best regards,
> Javier Carrasco

-- 
Thanks,
Jai

GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Abdel Alkuor]

On Fri, Jan 05, 2024 at 03:40:54PM +0530, Jai Luthra wrote:
Hi Jai and Jvaier,
> > My biggest concern is that we are sending GAID for the tps25750 under
> > the same circumstances. Could we not have the same problem with that
> > device? We would be resetting the PD controller and the SoC would stop
> > getting power as well, right? Or is there anything device-specific that
> > would avoid that?
> > 
> 
> Yes I would guess same problem can happen depending on probe order of 
> the remote-endpoint node, but I don't see any upstream platform using 
> ti,tps25750 compatible, so I have no way to confirm.
> 
> Maybe Abdel can comment on how it works, as he added the GAID reset for 
> tps25750.
> 
Ops, that's an oversight from my side. In our case, fwnode_usb_role_switch_get()
returns NULL but if it does return -EPROBE_DEFER, we will end up with
the same issue you're seeing.

The purpose of the reset is to remove any applied patch so we don't
leave USB-C PD controller in some kind of operable state when the device
fails to be probed. GO2P command forces PD controller to retrun to PTCH
mode but unfortunately that doesn't work in all cases unless ADCINx pins
configurations are set in "NegotiateHighVoltage" option, so I opted into 
using the hard reset instead regardless of ADCINx configurations.

> > > If you have a better architecture in mind that can reset only when PTCH 
> > > has been applied and not for other probe defers, feel free to send it on 
> > > top of it.
> > > 
> > 
> > I added the cold reset to have the same behavior upon probe failures for
> > both devices, given that they use the same command. But if that can
> > cause problems, let's leave the reset alone...
> > 
I think in this case, we might want to apply the patch as the last
thing in the probe or check for EPROBE_DEFER before issuing a hard
reset.

Also, I think if the patch is being applied using EEPROM, I don't
believe we need to issue a hard reset ever as the patch would be applied
automatically after that.

Thanks,
Abdel