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

[PATCH 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>
---
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     | 151 ++++++++++++++++++++++++++++++++------
 drivers/usb/typec/tipd/tps6598x.h |  18 +++++
 2 files changed, 148 insertions(+), 21 deletions(-)
---
base-commit: 522c35e08b53f157ad3e51848caa861b258001e4
change-id: 20231207-tps6598x_update-632eab69d2ed

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

[PATCH 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

[PATCH 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.

A probe deferring mechanism has been added in order to account for cases
where the file system where the firmware resides is still not available
when the probe function is triggered and no firmware is found.

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

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index f0c4cd571a37..165a1391dc72 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -873,6 +873,31 @@ 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);
+		/* probe deferring in case the file system is not ready */
+		return (ret == -ENOENT) ? -EPROBE_DEFER : 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 +986,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 2/4] usb: typec: tipd: add function to request firmware

[Heikki Krogerus]

Hi Javier,

On Thu, Dec 07, 2023 at 12:51:07PM +0100, Javier Carrasco wrote:
> The firmware request process is device agnostic and can be used for
> other parts.
> 
> A probe deferring mechanism has been added in order to account for cases
> where the file system where the firmware resides is still not available
> when the probe function is triggered and no firmware is found.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  drivers/usb/typec/tipd/core.c | 36 +++++++++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index f0c4cd571a37..165a1391dc72 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -873,6 +873,31 @@ 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);
> +		/* probe deferring in case the file system is not ready */
> +		return (ret == -ENOENT) ? -EPROBE_DEFER : ret;

It's more likely that the firmware really isn't available, and it will
never be available in this case. I think there is only one place in
kernel where failing request_firmware() can lead to deferred probe
(drivers/tee/optee/smc_abi.c) and there the code can actually see the
system state - that's actually the condition.

So just return dev_err_probe() here:

	ret = request_firmware(fw, firmware_name, tps->dev);
	if (ret)
                return dev_err_probe(tps->dev, ret, "failed to retrieve \"%s\"", firmware_name);

> +	}
> +
> +	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 +986,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

thanks,

-- 
heikki

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

[Javier Carrasco]

Hi Heikki,

On 08.12.23 15:55, Heikki Krogerus wrote:

>> +	ret = request_firmware(fw, firmware_name, tps->dev);
>> +	if (ret) {
>> +		dev_err(tps->dev, "failed to retrieve \"%s\"\n", firmware_name);
>> +		/* probe deferring in case the file system is not ready */
>> +		return (ret == -ENOENT) ? -EPROBE_DEFER : ret;
> 
> It's more likely that the firmware really isn't available, and it will
> never be available in this case. I think there is only one place in
> kernel where failing request_firmware() can lead to deferred probe
> (drivers/tee/optee/smc_abi.c) and there the code can actually see the
> system state - that's actually the condition.
> 
> So just return dev_err_probe() here:
> 
> 	ret = request_firmware(fw, firmware_name, tps->dev);
> 	if (ret)
>                 return dev_err_probe(tps->dev, ret, "failed to retrieve \"%s\"", firmware_name);
> 
Thank you for your feedback.

This solution arose from a real use case: in the system I am using to
test the tps65987d, the filesystem is not ready when the probe function
is called. If I just return on -ENOENT, the device will never get the
update.

Note that we are only triggering the update if the device is in patch
mode, so a firmware will be expected for the device to run and reach the
app mode.
In that case deferring the probe and keeping on trying to make the
update makes sense because otherwise the device will not be able to
offer its functionality. If the device is not in patch mode, no update
will be triggered and the firmware will not be requested, so there will
not be any unnecessary probe deferring.

I see that the driver you mentioned checks if the system_state is still
not SYSTEM_RUNNING to defer the probe.
I have not tested if something like that would be possible in this case,
but giving up on the first attempt if the firmware is not found makes
the assumption that the filesystem where the fw resides will always be
ready when the probe function is called, which in my particular case is
a wrong assumption.

If the firmware was updated at any point during normal operation, the
assumption would be definitely right, but maybe not while booting.

Thank you and best regards,
Javier Carrasco

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

[Heikki Krogerus]

Hi,

On Fri, Dec 08, 2023 at 07:58:52PM +0100, Javier Carrasco wrote:
> Hi Heikki,
> 
> On 08.12.23 15:55, Heikki Krogerus wrote:
> 
> >> +	ret = request_firmware(fw, firmware_name, tps->dev);
> >> +	if (ret) {
> >> +		dev_err(tps->dev, "failed to retrieve \"%s\"\n", firmware_name);
> >> +		/* probe deferring in case the file system is not ready */
> >> +		return (ret == -ENOENT) ? -EPROBE_DEFER : ret;
> > 
> > It's more likely that the firmware really isn't available, and it will
> > never be available in this case. I think there is only one place in
> > kernel where failing request_firmware() can lead to deferred probe
> > (drivers/tee/optee/smc_abi.c) and there the code can actually see the
> > system state - that's actually the condition.
> > 
> > So just return dev_err_probe() here:
> > 
> > 	ret = request_firmware(fw, firmware_name, tps->dev);
> > 	if (ret)
> >                 return dev_err_probe(tps->dev, ret, "failed to retrieve \"%s\"", firmware_name);
> > 
> Thank you for your feedback.
> 
> This solution arose from a real use case: in the system I am using to
> test the tps65987d, the filesystem is not ready when the probe function
> is called. If I just return on -ENOENT, the device will never get the
> update.

Just like all the other devices that require firmware. This driver is
no different from the others, and it is also not the only one that
needs the firmware only in special cases. Just make the firmware part
of your ramdisk, or build the driver as a module.

Are these firmwares available linux-firmware (or are the going to be)?
https://git.kernel.org/?p=linux/kernel/git/firmware/linux-firmware.git

thanks,

-- 
heikki

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

[Javier Carrasco]

On 12.12.23 15:15, Heikki Krogerus wrote:
> Hi,
> 
> On Fri, Dec 08, 2023 at 07:58:52PM +0100, Javier Carrasco wrote:
>> Hi Heikki,
>>
>> On 08.12.23 15:55, Heikki Krogerus wrote:
>>
>>>> +	ret = request_firmware(fw, firmware_name, tps->dev);
>>>> +	if (ret) {
>>>> +		dev_err(tps->dev, "failed to retrieve \"%s\"\n", firmware_name);
>>>> +		/* probe deferring in case the file system is not ready */
>>>> +		return (ret == -ENOENT) ? -EPROBE_DEFER : ret;
>>>
>>> It's more likely that the firmware really isn't available, and it will
>>> never be available in this case. I think there is only one place in
>>> kernel where failing request_firmware() can lead to deferred probe
>>> (drivers/tee/optee/smc_abi.c) and there the code can actually see the
>>> system state - that's actually the condition.
>>>
>>> So just return dev_err_probe() here:
>>>
>>> 	ret = request_firmware(fw, firmware_name, tps->dev);
>>> 	if (ret)
>>>                 return dev_err_probe(tps->dev, ret, "failed to retrieve \"%s\"", firmware_name);
>>>
>> Thank you for your feedback.
>>
>> This solution arose from a real use case: in the system I am using to
>> test the tps65987d, the filesystem is not ready when the probe function
>> is called. If I just return on -ENOENT, the device will never get the
>> update.
> 
> Just like all the other devices that require firmware. This driver is
> no different from the others, and it is also not the only one that
> needs the firmware only in special cases. Just make the firmware part
> of your ramdisk, or build the driver as a module.

I wonder why then there is no general solution that does not force the
driver to be built as a module. If there is none, the documentation
should mention that somehow (sorry if it does, I missed it). Actually a
solution like the one implemented in the driver you mentioned could be
used by any driver that can wait to be updated when the system is
running.

> Are these firmwares available linux-firmware (or are the going to be)?
> https://git.kernel.org/?p=linux/kernel/git/firmware/linux-firmware.git
> 
> thanks,
> 
The firmware (at least for the tps6598x) can be tailored with a TI
specific tool and it depends on the use case, so I suppose making it
public does not make much sense.

Best regards,
Javier Carrasco

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

[Heikki Krogerus]

On Tue, Dec 12, 2023 at 03:41:35PM +0100, Javier Carrasco wrote:
> 
> 
> On 12.12.23 15:15, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Fri, Dec 08, 2023 at 07:58:52PM +0100, Javier Carrasco wrote:
> >> Hi Heikki,
> >>
> >> On 08.12.23 15:55, Heikki Krogerus wrote:
> >>
> >>>> +	ret = request_firmware(fw, firmware_name, tps->dev);
> >>>> +	if (ret) {
> >>>> +		dev_err(tps->dev, "failed to retrieve \"%s\"\n", firmware_name);
> >>>> +		/* probe deferring in case the file system is not ready */
> >>>> +		return (ret == -ENOENT) ? -EPROBE_DEFER : ret;
> >>>
> >>> It's more likely that the firmware really isn't available, and it will
> >>> never be available in this case. I think there is only one place in
> >>> kernel where failing request_firmware() can lead to deferred probe
> >>> (drivers/tee/optee/smc_abi.c) and there the code can actually see the
> >>> system state - that's actually the condition.
> >>>
> >>> So just return dev_err_probe() here:
> >>>
> >>> 	ret = request_firmware(fw, firmware_name, tps->dev);
> >>> 	if (ret)
> >>>                 return dev_err_probe(tps->dev, ret, "failed to retrieve \"%s\"", firmware_name);
> >>>
> >> Thank you for your feedback.
> >>
> >> This solution arose from a real use case: in the system I am using to
> >> test the tps65987d, the filesystem is not ready when the probe function
> >> is called. If I just return on -ENOENT, the device will never get the
> >> update.
> > 
> > Just like all the other devices that require firmware. This driver is
> > no different from the others, and it is also not the only one that
> > needs the firmware only in special cases. Just make the firmware part
> > of your ramdisk, or build the driver as a module.
> 
> I wonder why then there is no general solution that does not force the
> driver to be built as a module.

Why would you need anything like that? Are you saying that even if you
put the firmware into your ramdisk, the driver still fails to find the
firmware if it's statically build? If so, then there is something else
wrong.

> If there is none, the documentation
> should mention that somehow (sorry if it does, I missed it). Actually a
> solution like the one implemented in the driver you mentioned could be
> used by any driver that can wait to be updated when the system is
> running.
> 
> > Are these firmwares available linux-firmware (or are the going to be)?
> > https://git.kernel.org/?p=linux/kernel/git/firmware/linux-firmware.git
> > 
> > thanks,
> > 
> The firmware (at least for the tps6598x) can be tailored with a TI
> specific tool and it depends on the use case, so I suppose making it
> public does not make much sense.

Okay.

thanks,

-- 
heikki

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

[Javier Carrasco]

On 13.12.23 16:15, Heikki Krogerus wrote:
> On Tue, Dec 12, 2023 at 03:41:35PM +0100, Javier Carrasco wrote:
>> I wonder why then there is no general solution that does not force the
>> driver to be built as a module.
> 
> Why would you need anything like that? Are you saying that even if you
> put the firmware into your ramdisk, the driver still fails to find the
> firmware if it's statically build? If so, then there is something else
> wrong.
> 
The firmware is always found unless the file system is still not ready,
which is the case on the system I am working on. If the driver is built
as a module, the issue is gone as expected.

My point was that there is no limitation to have the driver built-in and
no documentation to reference, so anyone could stumble on the same
issue. And as you said, this driver is not special in that sense, so
other drivers might be facing the same eventuality.
Am I missing any existing documentation for the fact that the firmware
must be put into the ramdisk or the driver must be built as a module? Or
is it only based on common sense?

Anyway the next version will not have any probe deferring and only
return an error if the firmware is not available.

Thanks and best regards,
Javier Carrasco

[PATCH 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 165a1391dc72..cd5214c9799e 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

[PATCH 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 cd5214c9799e..a4a50c52253d 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -1126,6 +1126,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;
@@ -1151,7 +1216,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)
@@ -1469,6 +1534,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