Commit 3f88b459 authored by Maximilian Luz's avatar Maximilian Luz Committed by Hans de Goede
Browse files

platform/surface: aggregator: Improve documentation and handling of message target and source IDs 



The `tid_in` and `tid_out` fields of the serial hub protocol command
struct (struct ssh_command) are actually source and target IDs,
indicating the peer from which the message originated and the peer for
which it is intended.

Change the naming of those fields accordingly and improve the protocol
documentation. Additionally, introduce an enum containing all currently
known peers, i.e. targets and sources.

Signed-off-by: default avatarMaximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20221202223327.690880-3-luzmaximilian@gmail.com


Reviewed-by: default avatarHans de Goede <hdegoede@redhat.com>
Signed-off-by: default avatarHans de Goede <hdegoede@redhat.com>
parent 070b3098

Documentation/driver-api/surface_aggregator/client.rst

+2 −2
Original line number Diff line number Diff line
@@ -191,7 +191,7 @@ data received from it is converted from little-endian to host endianness. 
            *       they do not correspond to an actual SAM/EC request.

            */

           rqst.target_category = SSAM_SSH_TC_SAM;

           rqst.target_id = 0x01;

           rqst.target_id = SSAM_SSH_TID_SAM;

           rqst.command_id = 0x02;

           rqst.instance_id = 0x03;

           rqst.flags = SSAM_REQUEST_HAS_RESPONSE;
@@ -241,7 +241,7 @@ one of the generator macros, for example via: 


   SSAM_DEFINE_SYNC_REQUEST_W(__ssam_tmp_perf_mode_set, __le32, {

           .target_category = SSAM_SSH_TC_TMP,

           .target_id       = 0x01,

           .target_id       = SSAM_SSH_TID_SAM,

           .command_id      = 0x03,

           .instance_id     = 0x00,

   });

Documentation/driver-api/surface_aggregator/ssh.rst

+19 −17
Original line number Diff line number Diff line
@@ -13,6 +13,7 @@ 
.. |DATA_NSQ| replace:: ``DATA_NSQ``

.. |TC| replace:: ``TC``

.. |TID| replace:: ``TID``

.. |SID| replace:: ``SID``

.. |IID| replace:: ``IID``

.. |RQID| replace:: ``RQID``

.. |CID| replace:: ``CID``
@@ -219,13 +220,13 @@ following fields, packed together and in order: 
     - |u8|

     - Target category.



   * - |TID| (out)

   * - |TID|

     - |u8|

     - Target ID for outgoing (host to EC) commands.

     - Target ID for commands/messages.



   * - |TID| (in)

   * - |SID|

     - |u8|

     - Target ID for incoming (EC to host) commands.

     - Source ID for commands/messages.



   * - |IID|

     - |u8|
@@ -286,19 +287,20 @@ general, however, a single target category should map to a single reserved 
event request ID.



Furthermore, requests, responses, and events have an associated target ID

(``TID``). This target ID is split into output (host to EC) and input (EC to

host) fields, with the respecting other field (e.g. output field on incoming

messages) set to zero. Two ``TID`` values are known: Primary (``0x01``) and

secondary (``0x02``). In general, the response to a request should have the

same ``TID`` value, however, the field (output vs. input) should be used in

accordance to the direction in which the response is sent (i.e. on the input

field, as responses are generally sent from the EC to the host).



Note that, even though requests and events should be uniquely identifiable

by target category and command ID alone, the EC may require specific

target ID and instance ID values to accept a command. A command that is

accepted for ``TID=1``, for example, may not be accepted for ``TID=2``

and vice versa.

(``TID``) and source ID (``SID``). These two fields indicate where a message

originates from (``SID``) and what the intended target of the message is

(``TID``). Note that a response to a specific request therefore has the source

and target IDs swapped when compared to the original request (i.e. the request

target is the response source and the request source is the response target).

See (:c:type:`enum ssh_request_id <ssh_request_id>`) for possible values of

both.



Note that, even though requests and events should be uniquely identifiable by

target category and command ID alone, the EC may require specific target ID and

instance ID values to accept a command. A command that is accepted for

``TID=1``, for example, may not be accepted for ``TID=2`` and vice versa. While

this may not always hold in reality, you can think of different target/source

IDs indicating different physical ECs with potentially different feature sets.





Limitations and Observations

drivers/platform/surface/aggregator/controller.c

+6 −6
Original line number Diff line number Diff line
@@ -994,7 +994,7 @@ static void ssam_handle_event(struct ssh_rtl *rtl, 


	item->rqid = get_unaligned_le16(&cmd->rqid);

	item->event.target_category = cmd->tc;

	item->event.target_id = cmd->tid_in;

	item->event.target_id = cmd->sid;

	item->event.command_id = cmd->cid;

	item->event.instance_id = cmd->iid;

	memcpy(&item->event.data[0], data->ptr, data->len);
@@ -1779,35 +1779,35 @@ EXPORT_SYMBOL_GPL(ssam_request_sync_with_buffer); 


SSAM_DEFINE_SYNC_REQUEST_R(ssam_ssh_get_firmware_version, __le32, {

	.target_category = SSAM_SSH_TC_SAM,

	.target_id       = 0x01,

	.target_id       = SSAM_SSH_TID_SAM,

	.command_id      = 0x13,

	.instance_id     = 0x00,

});



SSAM_DEFINE_SYNC_REQUEST_R(ssam_ssh_notif_display_off, u8, {

	.target_category = SSAM_SSH_TC_SAM,

	.target_id       = 0x01,

	.target_id       = SSAM_SSH_TID_SAM,

	.command_id      = 0x15,

	.instance_id     = 0x00,

});



SSAM_DEFINE_SYNC_REQUEST_R(ssam_ssh_notif_display_on, u8, {

	.target_category = SSAM_SSH_TC_SAM,

	.target_id       = 0x01,

	.target_id       = SSAM_SSH_TID_SAM,

	.command_id      = 0x16,

	.instance_id     = 0x00,

});



SSAM_DEFINE_SYNC_REQUEST_R(ssam_ssh_notif_d0_exit, u8, {

	.target_category = SSAM_SSH_TC_SAM,

	.target_id       = 0x01,

	.target_id       = SSAM_SSH_TID_SAM,

	.command_id      = 0x33,

	.instance_id     = 0x00,

});



SSAM_DEFINE_SYNC_REQUEST_R(ssam_ssh_notif_d0_entry, u8, {

	.target_category = SSAM_SSH_TC_SAM,

	.target_id       = 0x01,

	.target_id       = SSAM_SSH_TID_SAM,

	.command_id      = 0x34,

	.instance_id     = 0x00,

});

drivers/platform/surface/aggregator/ssh_msgb.h

+2 −2
Original line number Diff line number Diff line
@@ -189,8 +189,8 @@ static inline void msgb_push_cmd(struct msgbuf *msgb, u8 seq, u16 rqid, 


	__msgb_push_u8(msgb, SSH_PLD_TYPE_CMD);		/* Payload type. */

	__msgb_push_u8(msgb, rqst->target_category);	/* Target category. */

	__msgb_push_u8(msgb, rqst->target_id);		/* Target ID (out). */

	__msgb_push_u8(msgb, 0x00);			/* Target ID (in). */

	__msgb_push_u8(msgb, rqst->target_id);		/* Target ID. */

	__msgb_push_u8(msgb, SSAM_SSH_TID_HOST);	/* Source ID. */

	__msgb_push_u8(msgb, rqst->instance_id);	/* Instance ID. */

	__msgb_push_u16(msgb, rqid);			/* Request ID. */

	__msgb_push_u8(msgb, rqst->command_id);		/* Command ID. */

drivers/platform/surface/aggregator/ssh_request_layer.c

+6 −5
Original line number Diff line number Diff line
@@ -920,13 +920,14 @@ static void ssh_rtl_rx_command(struct ssh_ptl *p, const struct ssam_span *data) 
	 * Check if the message was intended for us. If not, drop it.

	 *

	 * Note: We will need to change this to handle debug messages. On newer

	 * generation devices, these seem to be sent to tid_out=0x03. We as

	 * host can still receive them as they can be forwarded via an override

	 * option on SAM, but doing so does not change tid_out=0x00.

	 * generation devices, these seem to be sent to SSAM_SSH_TID_DEBUG. We

	 * as host can still receive them as they can be forwarded via an

	 * override option on SAM, but doing so does not change the target ID

	 * to SSAM_SSH_TID_HOST.

	 */

	if (command->tid_out != 0x00) {

	if (command->tid != SSAM_SSH_TID_HOST) {

		rtl_warn(rtl, "rtl: dropping message not intended for us (tid = %#04x)\n",

			 command->tid_out);

			 command->tid);

		return;

	}