usb: typec: ucsi: Add support for SET_PDOS command

Add support for updating message out data structure for UCSI ACPI interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and LPM Firmware Update.

Additionally, update ucsi_send_command to accept message_out data and .sync_control function to pass message_out data to write_message_out function if the command is UCSI_SET_PDOS.

Reviewed-by Heikki Krogerus <[email protected]>

Signed-off-by Pooja Katiyar <[email protected]>

---
Changelog v2:
- Moved write_message_out operation to .sync_control.
- Updated ucsi_send_command to accept message_out data.

  drivers/usb/typec/ucsi/cros_ec_ucsi.c |  4 +-
  drivers/usb/typec/ucsi/debugfs.c      |  4 +-
  drivers/usb/typec/ucsi/displayport.c  |  6 +--
  drivers/usb/typec/ucsi/ucsi.c         | 64 ++++++++++++++++-----------
  drivers/usb/typec/ucsi/ucsi.h         | 10 +++--
  drivers/usb/typec/ucsi/ucsi_acpi.c    | 20 ++++++++-
  drivers/usb/typec/ucsi/ucsi_ccg.c     |  4 +-
  7 files changed, 72 insertions(+), 40 deletions(-)

drivers/usb/typec/ucsi/ucsi.c +0 -0

  {
  	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
  	int ret;

@@ -68,6 +68,18 @@ int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
 

drivers/usb/typec/ucsi/ucsi.c +4 -0

+		if (ret)
+			goto out_clear_bit;
+	}
+
  	ret = ucsi->ops->async_control(ucsi, command);
  	if (ret)
  		goto out_clear_bit;

drivers/usb/typec/ucsi/ucsi_acpi.c +3 -0

index 6b92f296e985..c0426bb1e81b 100644
@@ -86,6 +86,21 @@ static int ucsi_acpi_read_message_in(struct ucsi *ucsi, void *val, size_t val_le
  	return 0;
  }
 
+static int ucsi_acpi_write_message_out(struct ucsi *ucsi, void *data, size_t data_len)
+{
+	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);

drivers/usb/typec/ucsi/ucsi_acpi.c +2 -0

+}
+
  static int ucsi_acpi_async_control(struct ucsi *ucsi, u64 command)
  {
  	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);

Quoted 15 lines

drivers/usb/typec/ucsi/ucsi_acpi.c +8 -0

index 6b92f296e985..c0426bb1e81b 100644
@@ -86,6 +86,21 @@ static int ucsi_acpi_read_message_in(struct ucsi *ucsi, void *val, size_t val_le
  	return 0;
  }
+static int ucsi_acpi_write_message_out(struct ucsi *ucsi, void *data, size_t data_len)
+{
+	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
+
+	if (!data || !data_len)
+		return -EINVAL;
+
+	if (ucsi->version <= UCSI_VERSION_1_2)

drivers/usb/typec/ucsi/ucsi_acpi.c static int ucsi_acpi_read_message_in(struct ucsi *ucsi, void *val, size_t val_le · line 101

Quoted 5 lines

drivers/usb/typec/ucsi/ucsi_acpi.c +5 -0

+		memcpy(ua->base + UCSI_MESSAGE_OUT, data, data_len);
+	else
+		memcpy(ua->base + UCSIv2_MESSAGE_OUT, data, data_len);
+
+	return ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_WRITE);

Why do we need extra WRITE? Isn't the one performed by ucsi_acpi_async_control() not enough?

Add support for updating message out data structure for UCSI ACPI interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and LPM Firmware Update.

Additionally, update ucsi_send_command to accept message_out data and .sync_control function to pass message_out data to write_message_out function if the command is UCSI_SET_PDOS.

Quoted 7 lines · Pooja Katiyar

Add support for updating message out data structure for UCSI ACPI interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and LPM Firmware Update.

Additionally, update ucsi_send_command to accept message_out data and .sync_control function to pass message_out data to write_message_out function if the command is UCSI_SET_PDOS.

Normally when you say "additionally" that implies that the patch should be split up into pieces. Why not do that here?

And do you _really_ need to add a new parameter to all of these functions? It's now getting even worse, look at this:

drivers/usb/typec/ucsi/debugfs.c static int ucsi_cmd(void *data, u64 val) · line 55

Quoted 4 lines

drivers/usb/typec/ucsi/debugfs.c +1 -1

 		ret = ucsi_send_command(ucsi, val,
 					&ucsi->debugfs->response,
-					sizeof(ucsi->debugfs->response));
+					sizeof(ucsi->debugfs->response), NULL);

You can kind of guess what the parameters mean before the NULL change, but now you have to go look up "what is the last pointer for" everywhere.

This feels very fragile and horrible to maintain over time, please reconsider this type of api change.

Hi,

Quoted 24 lines · Greg KH

Quoted 7 lines · Pooja Katiyar

Add support for updating message out data structure for UCSI ACPI interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and LPM Firmware Update.

Additionally, update ucsi_send_command to accept message_out data and .sync_control function to pass message_out data to write_message_out function if the command is UCSI_SET_PDOS.

Normally when you say "additionally" that implies that the patch should be split up into pieces. Why not do that here?

And do you _really_ need to add a new parameter to all of these functions? It's now getting even worse, look at this:

drivers/usb/typec/ucsi/debugfs.c static int ucsi_cmd(void *data, u64 val) · line 55

Quoted 4 lines

ret = ucsi_send_command(ucsi, val,
&ucsi->debugfs->response,

drivers/usb/typec/ucsi/debugfs.c +1 -1

-					sizeof(ucsi->debugfs->response));
+					sizeof(ucsi->debugfs->response), NULL);

You can kind of guess what the parameters mean before the NULL change, but now you have to go look up "what is the last pointer for" everywhere.

Quoted 2 lines

This feels very fragile and horrible to maintain over time, please reconsider this type of api change.

So I think what Pooja was proposing in the first version of this series, where you had a dedicated function for filling the message_out, was better after all.

Pooja, I'm really sorry about this, but can you revert back to that, and send it as v3? Let's start over.

Quoted 34 lines · Heikki Krogerus

Hi,

Quoted 24 lines · Greg KH

Quoted 7 lines · Pooja Katiyar

Add support for updating message out data structure for UCSI ACPI interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and LPM Firmware Update.

Additionally, update ucsi_send_command to accept message_out data and .sync_control function to pass message_out data to write_message_out function if the command is UCSI_SET_PDOS.

Normally when you say "additionally" that implies that the patch should be split up into pieces. Why not do that here?

And do you _really_ need to add a new parameter to all of these functions? It's now getting even worse, look at this:

drivers/usb/typec/ucsi/debugfs.c static int ucsi_cmd(void *data, u64 val) · line 55

Quoted 4 lines

ret = ucsi_send_command(ucsi, val,
&ucsi->debugfs->response,

drivers/usb/typec/ucsi/debugfs.c +1 -1

-					sizeof(ucsi->debugfs->response));
+					sizeof(ucsi->debugfs->response), NULL);

You can kind of guess what the parameters mean before the NULL change, but now you have to go look up "what is the last pointer for" everywhere.

Quoted 2 lines

This feels very fragile and horrible to maintain over time, please reconsider this type of api change.

So I think what Pooja was proposing in the first version of this series, where you had a dedicated function for filling the message_out, was better after all.

Quoted 2 lines

Pooja, I'm really sorry about this, but can you revert back to that, and send it as v3? Let's start over.

But that breaks the sync_control logic - currently it is possible to handle the command in .sync_control completely. If for any reason we need to implement workarounds for commands using MESSAGE_OUT field, we'd have to introduce additional logic (which we just got rid of).

Quoted 35 lines · Dmitry Baryshkov

Quoted 34 lines · Heikki Krogerus

Hi,

Quoted 24 lines · Greg KH

Quoted 7 lines · Pooja Katiyar

Add support for updating message out data structure for UCSI ACPI interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and LPM Firmware Update.

Additionally, update ucsi_send_command to accept message_out data and .sync_control function to pass message_out data to write_message_out function if the command is UCSI_SET_PDOS.

Normally when you say "additionally" that implies that the patch should be split up into pieces. Why not do that here?

And do you _really_ need to add a new parameter to all of these functions? It's now getting even worse, look at this:

drivers/usb/typec/ucsi/debugfs.c static int ucsi_cmd(void *data, u64 val) · line 55

Quoted 4 lines

ret = ucsi_send_command(ucsi, val,
&ucsi->debugfs->response,

drivers/usb/typec/ucsi/debugfs.c +1 -1

-					sizeof(ucsi->debugfs->response));
+					sizeof(ucsi->debugfs->response), NULL);

You can kind of guess what the parameters mean before the NULL change, but now you have to go look up "what is the last pointer for" everywhere.

Quoted 2 lines

This feels very fragile and horrible to maintain over time, please reconsider this type of api change.

So I think what Pooja was proposing in the first version of this series, where you had a dedicated function for filling the message_out, was better after all.

Quoted 7 lines

Quoted 2 lines

Pooja, I'm really sorry about this, but can you revert back to that, and send it as v3? Let's start over.

But that breaks the sync_control logic - currently it is possible to handle the command in .sync_control completely. If for any reason we need to implement workarounds for commands using MESSAGE_OUT field, we'd have to introduce additional logic (which we just got rid of).

Okay. So how about a data structure for the entire mailbox that we can pass to these functions?

struct ucsi_mailbox {
         u32 cci;
         u64 control;
         void *message_in;
         size_t message_in_size;
         void *message_out;
         size_t message_out_size;
};

thanks,

Quoted 47 lines · Heikki Krogerus

Quoted 35 lines · Dmitry Baryshkov

Quoted 34 lines · Heikki Krogerus

Hi,

Quoted 24 lines · Greg KH

Quoted 7 lines · Pooja Katiyar

Add support for updating message out data structure for UCSI ACPI interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and LPM Firmware Update.

Additionally, update ucsi_send_command to accept message_out data and .sync_control function to pass message_out data to write_message_out function if the command is UCSI_SET_PDOS.

Normally when you say "additionally" that implies that the patch should be split up into pieces. Why not do that here?

And do you _really_ need to add a new parameter to all of these functions? It's now getting even worse, look at this:

drivers/usb/typec/ucsi/debugfs.c static int ucsi_cmd(void *data, u64 val) · line 55

Quoted 4 lines

ret = ucsi_send_command(ucsi, val,
&ucsi->debugfs->response,

drivers/usb/typec/ucsi/debugfs.c +1 -1

-					sizeof(ucsi->debugfs->response));
+					sizeof(ucsi->debugfs->response), NULL);

You can kind of guess what the parameters mean before the NULL change, but now you have to go look up "what is the last pointer for" everywhere.

Quoted 2 lines

This feels very fragile and horrible to maintain over time, please reconsider this type of api change.

So I think what Pooja was proposing in the first version of this series, where you had a dedicated function for filling the message_out, was better after all.

Quoted 7 lines

Quoted 2 lines

Pooja, I'm really sorry about this, but can you revert back to that, and send it as v3? Let's start over.

But that breaks the sync_control logic - currently it is possible to handle the command in .sync_control completely. If for any reason we need to implement workarounds for commands using MESSAGE_OUT field, we'd have to introduce additional logic (which we just got rid of).

Okay. So how about a data structure for the entire mailbox that we can pass to these functions?

Quoted 8 lines

struct ucsi_mailbox {
         u32 cci;
         u64 control;
         void *message_in;
         size_t message_in_size;
         void *message_out;
         size_t message_out_size;
};

What about a slightly different proposal (following ucsi_ccg design):

struct ucsi {
   // .....
   u32 cci;
   u8 message_in[UCSI_MAX_MESSAGE_IN];
   u8 message_out[UCSI_MAX_MESSAGE_OUT];
};

The caller will fill ucsi->message_out, call sync_control with a proper length specified, then read UCSI_CCI_LENGTH(ucsi->cci) bytes from ucsi->message_in.

Note: I'm positive that we can handle message buffers in this way. I'm not so sure about the CCI, it might be too dynamic.

Quoted 10 lines · Dmitry Baryshkov

Quoted 9 lines · Heikki Krogerus

Quoted 8 lines · Dmitry Baryshkov

Quoted 7 lines · Heikki Krogerus

Hi,

Quoted 4 lines · Greg KH

Quoted 3 lines · Pooja Katiyar

Add support for updating message out data structure for UCSI ACPI interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and LPM Firmware Update.

Quoted 22 lines

Quoted 11 lines

Okay. So how about a data structure for the entire mailbox that we can pass to these functions?

struct ucsi_mailbox {
         u32 cci;
         u64 control;
         void *message_in;
         size_t message_in_size;
         void *message_out;
         size_t message_out_size;
};

What about a slightly different proposal (following ucsi_ccg design):

struct ucsi {
   // .....
   u32 cci;
   u8 message_in[UCSI_MAX_MESSAGE_IN];
   u8 message_out[UCSI_MAX_MESSAGE_OUT];
};

Quoted 3 lines

The caller will fill ucsi->message_out, call sync_control with a proper length specified, then read UCSI_CCI_LENGTH(ucsi->cci) bytes from ucsi->message_in.

Looks better indeed.

Quoted 2 lines

Note: I'm positive that we can handle message buffers in this way. I'm not so sure about the CCI, it might be too dynamic.

Pooja, I'm sorry about the extra work needed. Can you check this?

P.S. I'm already on vacation so there will be delays with my responses.

Hello Greg, Heikki and Dmitry,

Quoted 11 lines · Heikki Krogerus

Quoted 10 lines · Dmitry Baryshkov

Quoted 9 lines · Heikki Krogerus

Quoted 8 lines · Dmitry Baryshkov

Quoted 7 lines · Heikki Krogerus

Hi,

Quoted 4 lines · Greg KH

Quoted 3 lines · Pooja Katiyar

Add support for updating message out data structure for UCSI ACPI interface for UCSI 2.1 and UCSI 3.0 commands such as Set PDOs and LPM Firmware Update.

...

Quoted 28 lines

Quoted 22 lines

Quoted 11 lines

Okay. So how about a data structure for the entire mailbox that we can pass to these functions?

struct ucsi_mailbox {
         u32 cci;
         u64 control;
         void *message_in;
         size_t message_in_size;
         void *message_out;
         size_t message_out_size;
};

What about a slightly different proposal (following ucsi_ccg design):

struct ucsi {
   // .....
   u32 cci;
   u8 message_in[UCSI_MAX_MESSAGE_IN];
   u8 message_out[UCSI_MAX_MESSAGE_OUT];
};

Quoted 3 lines

The caller will fill ucsi->message_out, call sync_control with a proper length specified, then read UCSI_CCI_LENGTH(ucsi->cci) bytes from ucsi->message_in.

Looks better indeed.

Quoted 8 lines

Quoted 2 lines

Note: I'm positive that we can handle message buffers in this way. I'm not so sure about the CCI, it might be too dynamic.

Pooja, I'm sorry about the extra work needed. Can you check this?

P.S. I'm already on vacation so there will be delays with my responses.

Thank you for the feedback and suggestions.

I can start working on the changes. Before diving into implementation, I would like to clarify a few points.

1. What is the preference with respect to the lengths of message in and message out, should the length be included as part of the ucsi structure or should be passed as separate parameters to relevant functions?

2. As Dmitriy pointed out, cci changes might be too dynamic and hence we believe it's better to include message_in and message_out as part of the ucsi struct and exclude cci. What are your thoughts on this?

3. Implementing these changes will affect functions across different interfaces such as ACPI, ucsi_ccg, cros_ec_ucsi etc. At this time, we can only validate these changes on the ACPI interface. Unfortunately, we don't have the means to test other interfaces. What is your suggestion to proceed with implementation? should owners of other interfaces be involved for validation?

Add debugfs entry for writing message_out data structure to handle UCSI 2.1 and 3.0 commands through debugfs interface.

Users writing to the message_out debugfs file should ensure the input data adheres to the following format:
1. Input must be a non-empty valid hexadecimal string.
2. Input length of hexadecimal string must not exceed 256 bytes of length to be in alignment with the message out data structure size as per the UCSI specification v2.1.
3. If the input string length is odd, then user needs to prepend a '0' to the first character for proper hex conversion.

Below are examples of valid hex strings. Note that these values are just examples. The exact values depend on specific command use case.

	#echo 1A2B3C4D > message_out
	#echo 01234567 > message_out

Reviewed-by Heikki Krogerus <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Signed-off-by Pooja Katiyar <[email protected]>

---
 drivers/usb/typec/ucsi/debugfs.c | 26 ++++++++++++++++++++++++++
 drivers/usb/typec/ucsi/ucsi.h    |  3 +++
 2 files changed, 29 insertions(+)

Add support for UCSI SET_PDOS command as per UCSI specification v2.1 and above to debugfs.

Reviewed-by Heikki Krogerus <[email protected]>

Signed-off-by Pooja Katiyar <[email protected]>

---
Changelog v2:
- Send message_out data as part of ucsi_send_command.