* [PATCH v1 01/12] Add documentation for Sahara protocol.
2025-08-25 10:19 [PATCH v1 00/12] Sahara protocol enhancements Kishore Batta
@ 2025-08-25 10:19 ` Kishore Batta
2025-08-25 10:19 ` [PATCH v1 02/12] drivers: accel : Move AIC specific image tables to mhi_controller.c file Kishore Batta
` (10 subsequent siblings)
11 siblings, 0 replies; 26+ messages in thread
From: Kishore Batta @ 2025-08-25 10:19 UTC (permalink / raw)
To: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio
Cc: andersson, linux-arm-msm
Introduce documentation for the Sahara protocol, describing its
operational modes and their respective functions. The image transfer mode
enables firmware transfer from host to device. The memory debug mode
allows extraction of device memory contents to host. The command mode
facilitates retrieval of DDR training data from the device and also
to restore the training data back to device in subsequent boot of device
to save boot time.
Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
---
Documentation/sahara/index.rst | 14 +
Documentation/sahara/sahara_protocol.rst | 1241 ++++++++++++++++++++++
2 files changed, 1255 insertions(+)
create mode 100644 Documentation/sahara/index.rst
create mode 100644 Documentation/sahara/sahara_protocol.rst
diff --git a/Documentation/sahara/index.rst b/Documentation/sahara/index.rst
new file mode 100644
index 000000000000..073002c15a20
--- /dev/null
+++ b/Documentation/sahara/index.rst
@@ -0,0 +1,14 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+========================
+Qualcomm Sahara protocol
+========================
+
+The Sahara protocol transfers data to and from memory and describes packet
+structures, packet flows, and their usage.
+
+.. toctree::
+ :maxdepth: 2
+ :caption: Contents
+
+ sahara_protocol
diff --git a/Documentation/sahara/sahara_protocol.rst b/Documentation/sahara/sahara_protocol.rst
new file mode 100644
index 000000000000..91204bb7d170
--- /dev/null
+++ b/Documentation/sahara/sahara_protocol.rst
@@ -0,0 +1,1241 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+
+=============================
+Sahara protocol Specification
+=============================
+
+The Qualcomm Sahara protocol driver is primarily designed for transferring
+software images from a host device to a target device using a simplified data
+transfer mechanism over a link. However, the sahara protocol does not support
+any authentication/validation of the data sent between devices. Such a mechanism
+is beyond the scope of the protocol.
+
+The Sahara protocol defines two types of packets - Command packet and Data
+packet.
+
+Command packet
+--------------
+ These packets are sent between the host and the target to setup transfers of
+ data packets. The command packets contain a command ID and packet length.
+ Depending on the command, the packet may contain additional command specific
+ field.
+
++-------------+---------------+----------------+----------------+
+| Command ID | Packet length | Optional field | Optional field |
++-------------+---------------+----------------+----------------+
+
+Data packet
+-----------
+ The data packets contain RAW data as shown below.
+
++---------------------------------------------------------+
+| RAW Data (arbitrary number of bytes) |
++---------------------------------------------------------+
+
+Command packet optional fields
+------------------------------
+
++---------+---------------+---------+-----------------------------------------+
+| ID val | Field | Sent by | Description |
++---------+---------------+---------+-----------------------------------------+
+| 0x0 | - | - | Invalid |
++---------+---------------+---------+-----------------------------------------+
+| 0x1 | Hello packet | Target | Initializes connection and protocol |
++---------+---------------+---------+-----------------------------------------+
+| 0x2 | Hello response| Host | Acknowledges connection and protocol |
+| | | | sent by target. Also used to set mode of|
+| | | | operation for target to execute. |
++---------+---------------+---------+-----------------------------------------+
+| 0x3 | Read data | Target | Reads specified number of bytes from |
+| | | | host for a given image. |
++---------+---------------+---------+-----------------------------------------+
+| 0x4 | End of image | Target | Indicates host that the single image tx |
+| | transfer | | is complete. Also used to indicate a |
+| | | | target failure during an image transfer |
++---------+---------------+---------+-----------------------------------------+
+| 0x5 | Done packet | Host | Sends acknowledgment from host that a |
+| | | | single image transfer is complete. |
++---------+---------------+---------+-----------------------------------------+
+| 0x6 | Done response | Target | Provides the following information to |
+| | | | host. |
+| | | | 1. Target is exiting protocol |
+| | | | 2. Whether the target expects to |
+| | | | re-enter protocol to transfer another |
+| | | | image. |
++---------+---------------+---------+-----------------------------------------+
+| 0x7 | Reset packet | Host | Instructs target to perform a reset. |
++---------+---------------+---------+-----------------------------------------+
+| 0x8 | Reset response| Target | Indicates host that target is about to |
+| | | | reset. |
++---------+---------------+---------+-----------------------------------------+
+| 0x9 | Memory debug | Target | Indicates host that target has entered |
+| | packet | | a debug mode where it is ready to |
+| | | | transfer its system memory contents |
++---------+---------------+---------+-----------------------------------------+
+| 0xA | Memory read | Host | Reads specified number of bytes from |
+| | packet | | target's system memory, starting from a |
+| | | | specified address. |
++---------+---------------+---------+-----------------------------------------+
+| 0xB | Command ready | Target | Indicates host that target is ready to |
+| | packet | | receive client commands. |
++---------+---------------+---------+-----------------------------------------+
+| 0xC | Command switch| Host | Indicates target to switch modes. |
+| | mode packet | | 1. Image transfer pending mode. |
+| | | | 2. Image transfer complete mode. |
+| | | | 3. Memory debug mode. |
+| | | | 4. Command mode. |
++---------+---------------+---------+-----------------------------------------+
+| 0xD | Command | Host | Indicates target to execute a given |
+| | execute packet| | client command. |
++---------+---------------+---------+-----------------------------------------+
+| 0xE | Command | Target | Indicates host that target has executed |
+| | execute | | client command. Also used to indicate |
+| | response | | status of executed command. |
+| | packet | | |
++---------+---------------+---------+-----------------------------------------+
+| 0xF | Command | Host | Indicates target that host is ready to |
+| | execute | | receive data resulting from executing |
+| | data | | previous client command. |
+| | packet | | |
++---------+---------------+---------+-----------------------------------------+
+| 0x10 | 64 bit Memory | Target | Indicates host that target has entered |
+| | debug packet | | a debug mode where it is ready to |
+| | | | transfer its 64 bit system memory |
+| | | | contents. |
++---------+---------------+---------+-----------------------------------------+
+| 0x11 | 64 bit Memory | Host | Reads specified number of bytes from |
+| | read packet | | target's system memory, starting from a |
+| | | | 64 bit specified address. |
++---------+---------------+---------+-----------------------------------------+
+| 0x12 | 64 bit Read | Target | Reads specified number of bytes from |
+| | data | | host for a given 64 bit image. |
++---------+---------------+---------+-----------------------------------------+
+| 0x13 | Reset sahara | Host | Resets Sahara state machine and enters |
+| | sate machine | | Sahara entry without target reset |
+| | packet | | |
++---------+---------------+---------+-----------------------------------------+
+| 0x14 | Write data | Target | Writes specified number of bytes to host|
+| | packet | | for a given image |
++---------+---------------+---------+-----------------------------------------+
+| Others | - | - | Invalid |
++---------+---------------+---------+-----------------------------------------+
+
+
+Hello Packet
+------------
+
+The hello packet is the first packet that the target sends to the host. If the
+host receives any other packet, it sends a reset command to the target. When the
+host receives a valid hello packet, it first verifies that the protocol running
+on the target is compatible with the protocol running on the host. If the
+protocol mismatch, the host sends a reset command to the target. The target uses
+the following format while sending a hello packet.
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+| Version | 4 | Version number of this protocol |
++-----------+-------------+--------------------------------------+
+| Version | 4 | Lowest Compatible version |
+| Compatible| | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Maximum command packet length |
+| packet | | (in bytes) the protocol supports. |
+| length | | |
++-----------+-------------+--------------------------------------+
+| Mode | 4 | Expected mode of target operation |
++-----------+-------------+--------------------------------------+
+| Reserved | 4 | Reserved for future use. |
++-----------+-------------+--------------------------------------+
+| Reserved | 4 | Reserved for future use. |
++-----------+-------------+--------------------------------------+
+| Reserved | 4 | Reserved for future use. |
++-----------+-------------+--------------------------------------+
+| Reserved | 4 | Reserved for future use. |
++-----------+-------------+--------------------------------------+
+| Reserved | 4 | Reserved for future use. |
++-----------+-------------+--------------------------------------+
+| Reserved | 4 | Reserved for future use. |
++-----------+-------------+--------------------------------------+
+
+The target also sends the following information:
+ 1. Maximum length of the command packet that it supports. The host uses this
+ information to avoid sending more bytes than the target can support in the
+ receiving command buffer.
+ 2. Mode of operation it expects to enter, based on the boot up sequence. The
+ supported modes of operation for the target are as follows:
+
++-----------------------------+---------+------------------------------------+
+| Mode | Mode ID | Description |
++-----------------------------+---------+------------------------------------+
+| SAHARA_MODE_IMAGE_TX_PENDING| 0x0 | Image transfer is in the pending |
+| | | mode. Transfer image from the host.|
+| | | After completion, the host should |
+| | | expect another image transfer |
+| | | request. |
++-----------------------------+---------+------------------------------------+
+|SAHARA_MODE_IMAGE_TX_COMPLETE| 0x1 | Image transfer is in the complete |
+| | | mode. Transfer image from the host.|
+| | | After completion, the host should |
+| | | not expect another image transfer |
+| | | request. |
++-----------------------------+---------+------------------------------------+
+| SAHARA_MODE_MEMORY_DBEUG | 0x2 | Memory debug mode. The host should |
+| | | prepare to receive a memory dump |
+| | | from the target. |
++-----------------------------+---------+------------------------------------+
+| SAHARA_MODE_COMMAND | 0x3 | Command mode. The host executes |
+| | | operations on the target by sending|
+| | | the appropriate client command to |
+| | | the sahara client running on the |
+| | | target. The Sahar client interprets|
+| | | the client command and the response|
+| | | is sent after execution of the |
+| | | given command. |
++-----------------------------+---------+------------------------------------+
+
+Hello response packet
+---------------------
+
+After the host validates the protocol running on the target, it sends a response
+to the target. The response contains the following information.
+1. The protocol version that is running.
+2. The minimum protocol version that it supports.
+3. The mode of operation.
+
+The host sets the packet status field to success if no errors occur on the host
+side. After the target receives this packet, it can proceed with data transfer
+requests or memory debug. The host uses the following format while sending a
+hello response packet.
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++===========+=============+======================================+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet (in bytes) |
++-----------+-------------+--------------------------------------+
+| Version | 4 | Version number of this protocol |
++-----------+-------------+--------------------------------------+
+| Compatible| 4 | Lowest Compatible version |
++-----------+-------------+--------------------------------------+
+| Status | 4 | Success or error code |
++-----------+-------------+--------------------------------------+
+| Mode | 4 | Mode of operation for target to |
+| | | execute |
++-----------+-------------+--------------------------------------+
+| Reserved | 4 | Reserved for future use |
++-----------+-------------+--------------------------------------+
+| Reserved | 4 | Reserved for future use |
++-----------+-------------+--------------------------------------+
+| Reserved | 4 | Reserved for future use |
++-----------+-------------+--------------------------------------+
+| Reserved | 4 | Reserved for future use |
++-----------+-------------+--------------------------------------+
+| Reserved | 4 | Reserved for future use |
++-----------+-------------+--------------------------------------+
+| Reserved | 4 | Reserved for future use |
++-----------+-------------+--------------------------------------+
+
+
+Read data packet / 64 bit read data packet
+------------------------------------------
+
+The read data packet serves as a generic data transfer packet when any image
+data is to be transferred from the host to the target. This packet allows
+flexibility in the way that the image is transferred from the host to the
+target. As the target controls which data gets transferred, the target can
+determine what parts of the image get transferred and in what order. The host
+need not be familiar about the structure of the image. It must open the file and
+start transferring the data to the target based on the parameters specified in
+the packet.
+
+This gives the target complete control over how the images are transferred and
+processed. To initiate an image transfer, the target fills the read data packet
+with the image ID corresponding to the image that it wants to receive. The
+target also sends the offset into the image file and the length of the data(in
+bytes) it wants to read from the image. After the host receives this packet, the
+host responds with a data packet, which contains image data with the length
+specified in the read data packet. The host uses the following format while
+transferring the read data packet and 64-bit read data packet.
+
+
+Read data packet format
+=======================
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+| Image ID | 4 | ID of the image to be transferred. |
++-----------+-------------+--------------------------------------+
+| Data | 4 | Offset into the image file to start |
+| offset | | transferring data. |
++-----------+-------------+--------------------------------------+
+| Data | 4 | Number of bytes target wants to |
+| Length | | transfer from the image. |
++-----------+-------------+--------------------------------------+
+
+
+64-bit read data packet format
+==============================
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+| Image ID | 8 | ID of the image to be transferred. |
++-----------+-------------+--------------------------------------+
+| Data | 8 | Offset into the image file to start |
+| offset | | transferring data. |
++-----------+-------------+--------------------------------------+
+| Data | 8 | Number of bytes target wants to |
+| Length | | transfer from the image. |
++-----------+-------------+--------------------------------------+
+
+If any of the preceding fields are invalid, or if any other error occurs on the
+host, the host sends a data packet with length that does not match with what the
+target was expecting. The resulting error forces the target to send an end of
+image transfer packet with an error code in the status field and enables both
+the target and the host to enter an error handling state.
+
+End of Image transfer packet
+----------------------------
+
+If an image transfer is successfully completed, the target sends the host an end
+of image transfer packet with a success status. The target then waits for the
+host to send a done packet. If any error occurs during the transfer or
+processing of the image data, the status is set to the corresponding error code,
+and the target waits for a different command packet.
+
+The host uses the following format while transferring end of image transfer
+packet:
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+| Image ID | 4 | ID of the image that was being |
+| | | transferred. |
++-----------+-------------+--------------------------------------+
+| Status | 4 | Success or error code |
++-----------+-------------+--------------------------------------+
+
+Done packet
+-----------
+
+If the host receives an end of image transfer packet with a success status, the
+host sends a done packet to indicate the target that it can exit the protocol
+and continue execution of code. The host uses the following format while sending
+the done packet:
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+
+To transfer another image from the host, the target must re-initiate the
+protocol by starting with another hello packet.
+
+Done Response packet
+--------------------
+
+If the target receives a done packet, it responds with a done response packet
+containing the image transfer status. The target uses the following format while
+sending the done response packet:
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+| Image Tx | 4 | Indicates whether target is |
+| Status | | expecting to receive another image |
+| | | or not. |
++-----------+-------------+--------------------------------------+
+
+If all the images are transferred, the target sends a complete status to enable
+the host to exit the protocol. If all the images are not transferred, the target
+sends a pending status and waits for another hello packet to arrive.
+
+Reset Packet
+------------
+
+The host sends a reset packet to reset the target. The target services a reset
+request only if its in a state where reset requests are valid. If the target
+receives an invalid reset request, the target sends an error in an end of image
+transfer packet. The format of reset packet is as follows:
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+
+
+Reset response packet
+---------------------
+
+If the target receives a valid reset request, it sends a reset response packet
+just before it resets. The purpose of this response is to acknowledge the host
+that the target received the reset request. The format of reset response packet
+is as follows:
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+
+
+Memory debug packet
+-------------------
+
+The target initiates a memory dump by sending the host a memory debug packet.
+This packet contains the address and length of the memory debug table. The
+memory debug table is a listing of memory locations that can be accessed and
+dumped to the host. The target uses the following format while sending the
+memory debug packet:
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+| Memory | 4 | Target sets this field to the address|
+| table | | in memory that stores the memory |
+| Address | | debug table. |
++-----------+-------------+--------------------------------------+
+| Memory | 4 | Length in bytes of memory debug |
+| table | | table. |
+| Length | | |
++-----------+-------------+--------------------------------------+
+
+Given the memory table address and length, the host issues a memory read to
+retrieve the table. After the host receives the memory table information, it can
+decode each entry and issue memory read requests to dump each memory location.
+
+Memory read packet / 64-bit memory read packet
+----------------------------------------------
+
+The host issues memory read commands for each section of memory that it dumps.
+The host uses the following format while sending the memory read packet and 64
+bit memory read packet:
+
+Memory read packet format
+=========================
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+| Memory | 4 | Memory location to read. |
+| Address | | |
++-----------+-------------+--------------------------------------+
+| Memory | 4 | Length in bytes of memory to read |
+| Length | | |
++-----------+-------------+--------------------------------------+
+
+64 bit memory read packet format
+================================
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+| Memory | 8 | Memory location to read. |
+| Address | | |
++-----------+-------------+--------------------------------------+
+| Memory | 8 | Length in bytes of memory to read |
+| Length | | |
++-----------+-------------+--------------------------------------+
+
+The accessible regions are defined in the memory debug table. For each memory
+read command received, the target verifies that the specified memory(address and
+length) is accessible and responds with a raw data packet. The content and
+length of the raw data packet is the memory dump starting from the memory
+address and length specified in the memory read packet. The memory debug table
+can also be read using a memory read command by setting the address and length
+to the values specified in the memory debug packet.
+
+If any error occurs on the target, an end of image transfer packet is sent with
+the corresponding error code and the host recognizes whether it is actual memory
+data or an end of image transfer packet. The host issues a reset command on
+completion of a successful memory dump. However, the protocol does not force
+this implementation.
+
+Command ready packet
+--------------------
+
+The target sends this packet to the host to indicate that the target is ready to
+execute client commands. The target uses the following format while sending the
+command ready packet:
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+
+
+Command switch mode packet
+--------------------------
+
+The host sends the command switch mode packet to the target so that the target
+can switch to another mode. The host uses the following format while sending the
+command switch mode packet:
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+| Mode | 4 | Mode of operation for target |
+| | | to execute. |
++-----------+-------------+--------------------------------------+
+
+Command execute packet
+----------------------
+
+The host sends this packet to execute the given client command on the target. If
+the client command successfully executes, the target sends a command execute
+response packet. If an error occurs, the target sends an end of image transfer
+packet with the corresponding error code. The host uses the following format
+while sending command execute packet:
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+| Client | 4 | Client Command to be executed. |
+| Command | | |
++-----------+-------------+--------------------------------------+
+
+
+Client commands
+===============
+
++------------+-------------+--------------------------------------+
+| Client ID | Length | Description |
++------------+-------------+--------------------------------------+
+| 0x8 | 4 | Get Command ID list. |
++------------+-------------+--------------------------------------+
+| 0x9 | 4 | Get DDR training data. |
++------------+-------------+--------------------------------------+
+
+Command execute Response packet
+-------------------------------
+
+The target sends this packet if it successfully executes the client command. The
+target uses the following format while sending the command execute response
+packet.
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+| Client | 4 | Client Command to be executed. |
+| Command | | |
++-----------+-------------+--------------------------------------+
+| Response | 4 | Number of bytes for response data. |
+| Length | | |
++-----------+-------------+--------------------------------------+
+
+Command execute data packet
+---------------------------
+
+The host sends this packet if the response length received in the command
+execute response packet is greater than 0. The host uses the following format
+while sending command execute data packet:
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+| Client | 4 | Client Command executed. |
+| Command | | |
++-----------+-------------+--------------------------------------+
+
+The packet indicates the target to send the response data in a raw data packet.
+The target sends the response data upon receiving this packet.
+
+64-bit memory debug packet
+--------------------------
+
+The target sends this packet to the host to initiate a memory dump. The packet
+contains 64-bit address and length of the memory table. The target uses the
+following format while sending 64-bit memory debug packet:
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+| Memory | 8 | Target sets this field to the 64-bit |
+| table | | address in memory that stores the |
+| Address | | memory debug table. |
++-----------+-------------+--------------------------------------+
+| Memory | 8 | Length in bytes of memory debug |
+| table | | table. |
+| Length | | |
++-----------+-------------+--------------------------------------+
+
+Reset Sahara state machine packet
+---------------------------------
+
+The host sends a reset sahara state machine packet whenever it wants to reset
+Sahara state machine. When the target receives a reset sahara state machine
+request, it reinitializes sahara protocol and sends the hello packet to the
+host. The sahara protocol is restarted without a target reset. The host uses the
+following format while sending the reset sahara state machine packet:
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+
+Write data packet
+-----------------
+
+Write data packet serves as a generic data transfer packet when any data is
+transferred from the target to the host. This packet allows flexible data
+transfer from the target to the host.
+
+As the target controls what data gets transferred, target can determine what
+parts of the data get transferred and in what order. The host does not need to
+know anything about the structure of the data. It only needs to open the file
+and start accepting the data to the host based on the parameters specified in
+the packet.
+
+To initiate a write data transfer, the target fills the write data packet with
+the image ID corresponding to the image data that it wants to send. The target
+also sends the offset into the output file and the length of the data(in bytes)
+it wants to write from the target. As soon as the host receives the packet, the
+host opens an output file and waits to receive the data packets. After the
+packet is received, the content from the data pcket is written to the output
+file, The format of the write data packet is as follows:
+
++-----------+-------------+--------------------------------------+
+| Field | Length | Description |
+| | (bytes) | |
++-----------+-------------+--------------------------------------+
+| Command | 4 | Command identifier code |
++-----------+-------------+--------------------------------------+
+| Length | 4 | Length of the packet(in bytes) |
++-----------+-------------+--------------------------------------+
+| Data | 8 | Offset into the image file to start |
+| offset | | writing the data to host. |
++-----------+-------------+--------------------------------------+
+| Image ID | 4 | ID of the image to be transferred. |
++-----------+-------------+--------------------------------------+
+| Data | 4 | Number of bytes target wants to |
+| Length | | transfer the data to the host. |
++-----------+-------------+--------------------------------------+
+
+
+Command packet flow between host and target
+-------------------------------------------
+
+Packet flow is a process of exchange of information as packets between the host
+and the target in a specific way using command packets. The sahara protocol
+allows packet processing for the following scenarios:
+
+1. Transferring an image from the host to the target.
+2. Dumping memory from the target to the host.
+3. Loading DDR calibration data on flashless target.
+
+Packet flow for Image transfer
+------------------------------
+
+The packet flow is performed between the host and target for a successful image
+transfer.
+
+.. code-block:: text
+
+ Host Target
+ | HELLO |
+ | (mode = image transfer) |
+ |<--------------------------|
+ | |
+ | HELLO RESP |
+ | (mode = image transfer) |
+ |-------------------------->|
+ | |
+ | READ_DATA |
+ | (img ID, 0, offset, |
+ | size of image header) |
+ |<--------------------------|
+ | |
+ | RAW_DATA |
+ | (size of image header) |
+ |-------------------------->|
+ | |
+ | READ_DATA |
+ | (img ID, segment 0 offset,|
+ | size of segment 0) |
+ |<--------------------------|
+ | RAW_DATA |
+ | (size of segment 0) |
+ |-------------------------->|
+ | |
+ | READ_DATA |
+ | (img ID, segment 1 offset,|
+ | size of segment 1) |
+ |<--------------------------|
+ | |
+ | |
+ | RAW_DATA |
+ | (size of segment 1) |
+ |-------------------------->|
+ | ... |
+ | ... |
+ | ... |
+ | ... |
+ | |
+ | |
+ | READ_DATA |
+ | (img ID, segment N offset,|
+ | size of segment N) |
+ |<--------------------------|
+ | |
+ | |
+ | |
+ | RAW_DATA |
+ | (size of segment N) |
+ |-------------------------->|
+ | |
+ | |
+ | END_IMAGE_TX |
+ |<--------------------------|
+ | |
+ | |
+ | DONE |
+ |-------------------------->|
+ | |
+ | |
+ | DONE_RESP |
+ |<--------------------------|
+ | |
+
+The packet flow sequence for image transfer is as follows:
+
+1. A hello packet is sent from the target to the host to initiate the protocol
+ with the mode set to either image transfer pending or image transfer
+ complete (depending on the target's boot sequence).
+
+2. The host sends a hello response packet with a success status and sets the
+ mode to the mode received in the hello packet. After it receives the hello
+ packet and validates the protocol version running on the target.
+
+3. After the target receives the hello response, the target initiates the
+ image transfer request by sending read data packets. Each read data packet
+ specifies the image that the target wishes to receive and what part of the
+ image is to be transferred.
+
+4. During normal operation, the target first requests image header information.
+
+ a. The image header information specifies image size and location of the
+ image data that is to be transferred.
+
+ b. The image header information (which is sent as a read data request)
+ allows the target to know the format of the image to be transferred.
+ The protocol does not require the host to know anything about the
+ image formats and allows the host to read and transfer data from the
+ image as requested by the target.
+
+ c. If the image is a standalone binary image without any data segmentation
+ (which means the data is entirely contiguous when stored as well as
+ transferred to the target system memory), then the target requests for
+ entire image data to be sent in one transfer.
+
+ d. If the image data is segmented and requires scattering of the data
+ segments to noncontiguous system memory locations, the target issues
+ multiple read data requests to enable each data segment to be
+ transferred directly to the respective destination address. This
+ scattered information resides in the image header and is parsed by the
+ target before issuing the read data requests.
+
+5. After receiving a read data request, the host parses the image ID, data
+ offset, and data length to transfer data from the corresponding image file.
+ The host sends the requested data without any packet header.
+
+6. The target directly transfers the data to the destination address without
+ any software processing or temporarily buffering of the data in system
+ memory by transferring the image header to the targert and setting the
+ receive buffer for the data as the destination address in system memory.
+
+7. After the target successfully receives all segments for an image, the
+ target sends an end of image transfer packet with the image ID of the
+ corresponding image, and a success status. The host stops reading and
+ closes the image file after receiving the success status.
+
+8. The host sends a done packet to allow the target to exit the protocol after
+ it receives a successgul end of image transfer packet.
+
+9. After the target receives the done packet, the target sends a done response
+ packet to the host. This packet indicates if the target expects another
+ image to be transferred and if the host can continue to run the protocol.
+
+Packet flow for memory debug
+----------------------------
+
+The packet flow is performed between the host and the target for the successful
+memory debug.
+
+.. code-block:: text
+
+ Host Target
+ | HELLO |
+ | (mode = memory debug) |
+ |<--------------------------|
+ | |
+ | HELLO RESP |
+ | (mode = memory debug) |
+ |-------------------------->|
+ | |
+ | MEMORY_DEBUG |
+ | (location of mem table, |
+ | size of memory table) |
+ |<--------------------------|
+ | |
+ | MEMORY_READ |
+ | (Address from region 0 ,|
+ | size of region 0) |
+ |-------------------------->|
+ | RAW_DATA |
+ | (size of region 0) |
+ |<--------------------------|
+ | |
+ | MEMORY_READ |
+ | (Address from region 1 ,|
+ | size of region 1) |
+ |-------------------------->|
+ | RAW_DATA |
+ | (size of region 1) |
+ |<--------------------------|
+ | MEMORY_READ |
+ | (Address from region 2 ,|
+ | size of region 0) |
+ |-------------------------->|
+ | RAW_DATA |
+ | (size of region 2) |
+ |<--------------------------|
+ | ... |
+ | ... |
+ | ... |
+ | ... |
+ | |
+ | MEMORY_READ |
+ | (Address from region N ,|
+ | size of region N) |
+ |-------------------------->|
+ | RAW_DATA |
+ | (size of region N) |
+ |<--------------------------|
+ | |
+ | RESET |
+ |-------------------------->|
+ | |
+ | |
+ | RESET_RESP |
+ |<--------------------------|
+ | |
+
+The packet flow sequence for image transfer is as follows:
+
+1. A hello packet is sent from the target to the host to initiate the protocol
+ with mode set to memory debug.
+
+2. The host sends a hello response packet with a success status and sets the
+ mode to memory debug after it receives the hello packet and validates the
+ protocol version running on the target.
+
+3. After the target receives the hello response, the target initiates the
+ memory dump by sending a memory debug packet with the location and size of
+ the memory debug table. The memory debug table specifies accessible memory
+ regions.
+
+4. The host then initiates a memory read packet to read the memory debug
+ table and receives the table in a raw data packet after it receives the
+ memory debug packet.
+
+5. The host then decodes the table and issues memory reads for each accessible
+ region. The data for each region is sent in a raw data packet.
+
+6. Upon completion, the host issues a reset to the target. The target sends a
+ reset response and resets the target.
+
+7. The host can alternatively send a command switch mode packet to allow the
+ target to switch modes and avoid a reset.
+
+
+Packet flow to load DDR calibration data on target
+--------------------------------------------------
+
+The packet flow is performed between the host and the target to load DDR
+calibration data on flashless target. This packet flow is initiated when the
+device boots up for the first time and needs DDR calibration. This packet flow
+is also initiated in other scenarios, such as build update or any reason for
+which DDR calibration data gets corrupted.
+
+First boot scenario or invalid calibration data in filesystem.
+--------------------------------------------------------------
+
+.. code-block:: text
+
+ Host Target
+ | HELLO |
+ | (mode = image transfer) |
+ |<--------------------------|
+ | |
+ | HELLO RESP |
+ | (mode = image transfer) |
+ |-------------------------->|
+ | |
+ | READ_DATA |
+ | (img ID:34, 0, offset, |
+ | size of DDR training data)|
+ |<--------------------------|
+ | |
+ | RAW_DATA |
+ |(size of DDR training data)|
+ |-------------------------->|
+ | |
+ | |
+ | END_IMAGE_TX |
+ |<--------------------------|
+ | |
+ | |
+ | DONE |
+ |-------------------------->|
+ | |
+ | |
+ | DONE_RESP |
+ | (mode = IMAGE_TX_PENDING) |
+ |<--------------------------|
+ |1. First boot scenario. |
+ | DDR driver performs |
+ | calibration and returns |
+ | to SBL. |
+ |2. Next: Push DDR |
+ | Calibration data to host |
+ | |
+ | |
+ | HELLO |
+ | (mode = COMMAND mode) |
+ |<--------------------------|
+ | |
+ | HELLO RESP |
+ | (mode = COMMAND mode ) |
+ |-------------------------->|
+ | |
+ | CMD_READY |
+ |<--------------------------|
+ | |
+ | CMD_EXEC |
+ |(cmd id = 8, Get command |
+ | ID to be executed) |
+ |-------------------------->|
+ | |
+ | CMD_EXEC_RESP |
+ |(cmd id: 8, resp len = 4) |
+ |<--------------------------|
+ | |
+ | CMD_EXEC_GET_DATA |
+ | (ID = 0x8) |
+ |-------------------------->|
+ | |
+ | RAW_DATA |
+ | (0x00000009) |
+ |<--------------------------|
+ | |
+ | CMD_EXEC |
+ | (cmd id: 9, resp len > 0) |
+ |-------------------------->|
+ | |
+ | |
+ | CMD_EXEC_RESP |
+ |(cmd id: 9, resp len > 0) |
+ |<--------------------------|
+ | |
+ | CMD_EXEC_GET_DATA |
+ | (ID = 0x9) |
+ |-------------------------->|
+ | |
+ | RAW_DATA |
+ | (valid training data) |
+ |<--------------------------|
+ | |
+ |3. Host sends switch to |
+ |image tx mode to continue |
+ |booting. |
+ | |
+ | |
+ | CMD_SWITCH_MODE |
+ | (mode = IMAGE_TX_PENDING) |
+ |-------------------------->|
+ | |
+ | |
+ | HELLO |
+ | (mode = IMAGE_TX_PENDING) |
+ |<--------------------------|
+ | |
+ | HELLO RESP |
+ | (mode = IMAGE_TX_PENDING) |
+ |-------------------------->|
+ | |
+ |4. Boot/Load rest of the |
+ | images.... |
+ | |
+ | END_IMAGE_TX |
+ |<--------------------------|
+ | |
+ | |
+ | DONE |
+ |-------------------------->|
+ | |
+ | |
+ | DONE_RESP |
+ |(mode = IMAGE_TX_COMPLETE) |
+ |<--------------------------|
+ | |
+
+The packet flow sequence is as follows :
+
+1. The target sends the hello packet to the host to initiate the protocol
+ with the mode set to image transfer pending.
+
+2. The host sends a hello response packet with a success status and sets the
+ mode to image transfer pending after it receives the hello packet and
+ validates the protocol version running on the target.
+
+3. After the target receives the hello response, it initiates the data
+ transfer by requesting the size of DDR training/calibration data.
+
+4. The host sends back the DDR training/calibration data to the target.
+
+5. The target decodes the training data and does not find valid DDR
+ calibration data, target sends END_IMAGE_TX to interrupt the transfer.
+
+6. The host sends DONE after receives END_IMAGE_TX.
+
+7. The target sends DONE_RESP with mode = IMAGE_TX_PENDING because it has
+ not received all images.
+
+8. The target executes DDR training process to generate valid DDR calibration
+ data and prepares to push back to host.
+
+9. The target initiates protocol by sending a hello packet with COMMAND_MODE
+ to the host.
+
+10. The host sends a hello response packet with a success status and sets the
+ mode to COMMAND_MODE.
+
+11. The target sends CMD_READY to the host.
+
+12. The host receives CMD_READY and starts to get command IDs to be executed.
+
+13. The target sends CMD_ID = 9 to push DDR calibration data to host.
+
+14. The host executes CMD_ID = 9 to get DDR calibration data from the target.
+
+15. The target sends RAW_DATA with the payload which contains DDR calibration
+ data to host.
+
+16. The host saves training data in the kernel buffer and exposes to userspace
+ via the sysfs entry. The host sends CMD_SWITCH_MODE with the mode set to
+ IMAGE_TX_PENDING to continue booting.
+
+17. After the target receives the CMD_SWITCH_MODE command, it sends HELLO to
+ the host with the mode set to IMAGE_TX_PENDING. The target and the host
+ repeat the packet flow for image transfer to get all booting-required
+ images.
+
+18. Upon successful transfer of all images, the target sends an END_IMAGE_TX
+ packet with a success status to the host.
+
+19. The host sends DONE after it receives END_IMAGE_TX.
+
+20. The target sends DONE_RESP with the mode set to IMAGE_TX_COMPLETE because
+ it has received all images. The process has been completed after the host
+ receives DONE_RESP with the mode set to IMAGE_TX_COMPLETE.
+
+Subsequent boot scenario with valid DDR calibration data
+--------------------------------------------------------
+
+The below firgure shows the subsequent boot scenario with valid DDR calibration
+data process being loaded from host to target.
+
+.. code-block:: text
+
+ Host Target
+ | HELLO |
+ | (mode = image transfer) |
+ |<--------------------------|
+ | |
+ | HELLO RESP |
+ | (mode = image transfer) |
+ |-------------------------->|
+ | |
+ | READ_DATA |
+ | (img ID:34, 0, offset, |
+ | size of DDR training data)|
+ |<--------------------------|
+ | |
+ | RAW_DATA |
+ |(size of DDR training data)|
+ |-------------------------->|
+ | |
+ | |
+ | END_IMAGE_TX |
+ |<--------------------------|
+ | |
+ | |
+ | DONE |
+ |-------------------------->|
+ | |
+ | |
+ | DONE_RESP |
+ | (mode = IMAGE_TX_PENDING) |
+ |<--------------------------|
+ | |
+ | Subsequent boot scenario |
+ | (valid calibration data) |
+ | DDR driver configures DDR |
+ | using valid calibration |
+ | data |
+ | |
+ | |
+ | HELLO |
+ | (mode = IMAGE_TX_PENDING) |
+ |<--------------------------|
+ | |
+ | HELLO RESP |
+ | (mode = IMAGE_TX_PENDING) |
+ |-------------------------->|
+ | |
+ | Boot/Load rest of the |
+ | images.... |
+ | |
+ | END_IMAGE_TX |
+ |<--------------------------|
+ | |
+ | |
+ | DONE |
+ |-------------------------->|
+ | |
+ | |
+ | DONE_RESP |
+ |(mode = IMAGE_TX_COMPLETE) |
+ |<--------------------------|
+ | |
+
+The packet flow is as follows :
+
+1. The target sends the hello packet to the host to initiate the protocol
+ with the mode set to image transfer pending.
+
+2. The host sends a hello response packet with a success status and sets the
+ mode to image transfer pending after it receives the hello packet and
+ validates the protocol version running on the target.
+
+3. After the target receives the hello response, it initiates the images
+ transfer by requesting the training/calibration data from the host.
+
+4. The host sends back the DDR training/calibration data to the target.
+
+5. The target decodes the DDR training/calibration data and finds valid DDR
+ calibration data.
+
+6. The host sends RAW_DATA with the size of the DDR calibration data to the
+ target.
+
+7. Upon successful transfer of DDR calibration data, the target sends an
+ END_IMAGE_TX packet with a success status.
+
+8. The host sends DONE after it receives END_IMAGE_TX.
+
+9. The target sends DONE_RESP with mode = IMAGE_TX_PENDING because it has not
+ received all images.
+
+10. The target continues booting with valid DDR calibration data.
+
+11. The target and the host repeat the packet flow for image transfer to get
+ all booting-required images.
+
+12. After successful transfer of all images, the target sends an END_IMAGE_TX
+ packet with a success status to the host.
+
+13. The host sends DONE after it receives END_IMAGE_TX.
+
+14. The target sends DONE_RESP with the mode set to IMAGE_TX_COMPLETE because
+ it has received all images. The process has been completed after the host
+ receives DONE_RESP with the mode set to IMAGE_TX_COMPLETE.
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v1 02/12] drivers: accel : Move AIC specific image tables to mhi_controller.c file
2025-08-25 10:19 [PATCH v1 00/12] Sahara protocol enhancements Kishore Batta
2025-08-25 10:19 ` [PATCH v1 01/12] Add documentation for Sahara protocol Kishore Batta
@ 2025-08-25 10:19 ` Kishore Batta
2025-08-25 21:08 ` Bjorn Andersson
2025-08-25 10:19 ` [PATCH v1 03/12] drivers: accel: qaic: Support for registration of image tables in Sahara Kishore Batta
` (9 subsequent siblings)
11 siblings, 1 reply; 26+ messages in thread
From: Kishore Batta @ 2025-08-25 10:19 UTC (permalink / raw)
To: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio
Cc: andersson, linux-arm-msm
Move the AIC-specific image tables from the Sahara driver to the AIC
specific controller file. This change prevents the Sahara driver from
being tagged to a specific Qualcomm device, making it easier to add
support for new devices with their own image tables.
Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
---
drivers/accel/qaic/mhi_controller.c | 43 +++++++++++++++++++++++++++++
drivers/accel/qaic/sahara.c | 43 ++---------------------------
drivers/accel/qaic/sahara.h | 7 +++++
3 files changed, 52 insertions(+), 41 deletions(-)
diff --git a/drivers/accel/qaic/mhi_controller.c b/drivers/accel/qaic/mhi_controller.c
index 13a14c6c6168..5cc7994f4809 100644
--- a/drivers/accel/qaic/mhi_controller.c
+++ b/drivers/accel/qaic/mhi_controller.c
@@ -790,6 +790,49 @@ static struct mhi_controller_config mhi_cntrl_configs[] = {
},
};
+const char * const aic100_image_table[] = {
+ [1] = "qcom/aic100/fw1.bin",
+ [2] = "qcom/aic100/fw2.bin",
+ [4] = "qcom/aic100/fw4.bin",
+ [5] = "qcom/aic100/fw5.bin",
+ [6] = "qcom/aic100/fw6.bin",
+ [8] = "qcom/aic100/fw8.bin",
+ [9] = "qcom/aic100/fw9.bin",
+ [10] = "qcom/aic100/fw10.bin",
+};
+
+const size_t aic100_image_table_size = ARRAY_SIZE(aic100_image_table);
+
+const char * const aic200_image_table[] = {
+ [5] = "qcom/aic200/uefi.elf",
+ [12] = "qcom/aic200/aic200-nsp.bin",
+ [23] = "qcom/aic200/aop.mbn",
+ [32] = "qcom/aic200/tz.mbn",
+ [33] = "qcom/aic200/hypvm.mbn",
+ [39] = "qcom/aic200/aic200_abl.elf",
+ [40] = "qcom/aic200/apdp.mbn",
+ [41] = "qcom/aic200/devcfg.mbn",
+ [42] = "qcom/aic200/sec.elf",
+ [43] = "qcom/aic200/aic200-hlos.elf",
+ [49] = "qcom/aic200/shrm.elf",
+ [50] = "qcom/aic200/cpucp.elf",
+ [51] = "qcom/aic200/aop_devcfg.mbn",
+ [57] = "qcom/aic200/cpucp_dtbs.elf",
+ [62] = "qcom/aic200/uefi_dtbs.elf",
+ [63] = "qcom/aic200/xbl_ac_config.mbn",
+ [64] = "qcom/aic200/tz_ac_config.mbn",
+ [65] = "qcom/aic200/hyp_ac_config.mbn",
+ [66] = "qcom/aic200/pdp.elf",
+ [67] = "qcom/aic200/pdp_cdb.elf",
+ [68] = "qcom/aic200/sdi.mbn",
+ [69] = "qcom/aic200/dcd.mbn",
+ [73] = "qcom/aic200/gearvm.mbn",
+ [74] = "qcom/aic200/sti.bin",
+ [75] = "qcom/aic200/pvs.bin",
+};
+
+const size_t aic200_image_table_size = ARRAY_SIZE(aic200_image_table);
+
static int mhi_read_reg(struct mhi_controller *mhi_cntrl, void __iomem *addr, u32 *out)
{
u32 tmp;
diff --git a/drivers/accel/qaic/sahara.c b/drivers/accel/qaic/sahara.c
index 3ebcc1f7ff58..cf8f8b585223 100644
--- a/drivers/accel/qaic/sahara.c
+++ b/drivers/accel/qaic/sahara.c
@@ -177,45 +177,6 @@ struct sahara_context {
bool is_mem_dump_mode;
};
-static const char * const aic100_image_table[] = {
- [1] = "qcom/aic100/fw1.bin",
- [2] = "qcom/aic100/fw2.bin",
- [4] = "qcom/aic100/fw4.bin",
- [5] = "qcom/aic100/fw5.bin",
- [6] = "qcom/aic100/fw6.bin",
- [8] = "qcom/aic100/fw8.bin",
- [9] = "qcom/aic100/fw9.bin",
- [10] = "qcom/aic100/fw10.bin",
-};
-
-static const char * const aic200_image_table[] = {
- [5] = "qcom/aic200/uefi.elf",
- [12] = "qcom/aic200/aic200-nsp.bin",
- [23] = "qcom/aic200/aop.mbn",
- [32] = "qcom/aic200/tz.mbn",
- [33] = "qcom/aic200/hypvm.mbn",
- [39] = "qcom/aic200/aic200_abl.elf",
- [40] = "qcom/aic200/apdp.mbn",
- [41] = "qcom/aic200/devcfg.mbn",
- [42] = "qcom/aic200/sec.elf",
- [43] = "qcom/aic200/aic200-hlos.elf",
- [49] = "qcom/aic200/shrm.elf",
- [50] = "qcom/aic200/cpucp.elf",
- [51] = "qcom/aic200/aop_devcfg.mbn",
- [57] = "qcom/aic200/cpucp_dtbs.elf",
- [62] = "qcom/aic200/uefi_dtbs.elf",
- [63] = "qcom/aic200/xbl_ac_config.mbn",
- [64] = "qcom/aic200/tz_ac_config.mbn",
- [65] = "qcom/aic200/hyp_ac_config.mbn",
- [66] = "qcom/aic200/pdp.elf",
- [67] = "qcom/aic200/pdp_cdb.elf",
- [68] = "qcom/aic200/sdi.mbn",
- [69] = "qcom/aic200/dcd.mbn",
- [73] = "qcom/aic200/gearvm.mbn",
- [74] = "qcom/aic200/sti.bin",
- [75] = "qcom/aic200/pvs.bin",
-};
-
static int sahara_find_image(struct sahara_context *context, u32 image_id)
{
int ret;
@@ -779,10 +740,10 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
if (!strcmp(mhi_dev->mhi_cntrl->name, "AIC200")) {
context->image_table = aic200_image_table;
- context->table_size = ARRAY_SIZE(aic200_image_table);
+ context->table_size = aic200_image_table_size;
} else {
context->image_table = aic100_image_table;
- context->table_size = ARRAY_SIZE(aic100_image_table);
+ context->table_size = aic100_image_table_size;
}
context->active_image_id = SAHARA_IMAGE_ID_NONE;
diff --git a/drivers/accel/qaic/sahara.h b/drivers/accel/qaic/sahara.h
index 640208acc0d1..d7fd447ca85b 100644
--- a/drivers/accel/qaic/sahara.h
+++ b/drivers/accel/qaic/sahara.h
@@ -7,4 +7,11 @@
int sahara_register(void);
void sahara_unregister(void);
+
+extern const char * const aic200_image_table[];
+extern const size_t aic200_image_table_size;
+
+extern const char * const aic100_image_table[];
+extern const size_t aic100_image_table_size;
+
#endif /* __SAHARA_H__ */
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v1 02/12] drivers: accel : Move AIC specific image tables to mhi_controller.c file
2025-08-25 10:19 ` [PATCH v1 02/12] drivers: accel : Move AIC specific image tables to mhi_controller.c file Kishore Batta
@ 2025-08-25 21:08 ` Bjorn Andersson
0 siblings, 0 replies; 26+ messages in thread
From: Bjorn Andersson @ 2025-08-25 21:08 UTC (permalink / raw)
To: Kishore Batta
Cc: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio,
linux-arm-msm
On Mon, Aug 25, 2025 at 03:49:16PM +0530, Kishore Batta wrote:
"git log --oneline -- drivers/accel/qaic/sahara.c" says that subject
prefix should be "accel/qaic: "
> Move the AIC-specific image tables from the Sahara driver to the AIC
> specific controller file. This change prevents the Sahara driver from
> being tagged to a specific Qualcomm device, making it easier to add
> support for new devices with their own image tables.
I don't have any concerns with moving the firmware mapping out of the
sahara driver, but the implementation already supports two different
devices...so it's not "tagged to a specific device".
Also, while at it, please start your commit message with a problem
statement and finish it with the technical description of the change
you're doing.
>
> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
> ---
> drivers/accel/qaic/mhi_controller.c | 43 +++++++++++++++++++++++++++++
> drivers/accel/qaic/sahara.c | 43 ++---------------------------
> drivers/accel/qaic/sahara.h | 7 +++++
> 3 files changed, 52 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/accel/qaic/mhi_controller.c b/drivers/accel/qaic/mhi_controller.c
> index 13a14c6c6168..5cc7994f4809 100644
> --- a/drivers/accel/qaic/mhi_controller.c
> +++ b/drivers/accel/qaic/mhi_controller.c
> @@ -790,6 +790,49 @@ static struct mhi_controller_config mhi_cntrl_configs[] = {
> },
> };
>
> +const char * const aic100_image_table[] = {
> + [1] = "qcom/aic100/fw1.bin",
> + [2] = "qcom/aic100/fw2.bin",
> + [4] = "qcom/aic100/fw4.bin",
> + [5] = "qcom/aic100/fw5.bin",
> + [6] = "qcom/aic100/fw6.bin",
> + [8] = "qcom/aic100/fw8.bin",
> + [9] = "qcom/aic100/fw9.bin",
> + [10] = "qcom/aic100/fw10.bin",
> +};
> +
> +const size_t aic100_image_table_size = ARRAY_SIZE(aic100_image_table);
> +
> +const char * const aic200_image_table[] = {
> + [5] = "qcom/aic200/uefi.elf",
> + [12] = "qcom/aic200/aic200-nsp.bin",
> + [23] = "qcom/aic200/aop.mbn",
> + [32] = "qcom/aic200/tz.mbn",
> + [33] = "qcom/aic200/hypvm.mbn",
> + [39] = "qcom/aic200/aic200_abl.elf",
> + [40] = "qcom/aic200/apdp.mbn",
> + [41] = "qcom/aic200/devcfg.mbn",
> + [42] = "qcom/aic200/sec.elf",
> + [43] = "qcom/aic200/aic200-hlos.elf",
> + [49] = "qcom/aic200/shrm.elf",
> + [50] = "qcom/aic200/cpucp.elf",
> + [51] = "qcom/aic200/aop_devcfg.mbn",
> + [57] = "qcom/aic200/cpucp_dtbs.elf",
> + [62] = "qcom/aic200/uefi_dtbs.elf",
> + [63] = "qcom/aic200/xbl_ac_config.mbn",
> + [64] = "qcom/aic200/tz_ac_config.mbn",
> + [65] = "qcom/aic200/hyp_ac_config.mbn",
> + [66] = "qcom/aic200/pdp.elf",
> + [67] = "qcom/aic200/pdp_cdb.elf",
> + [68] = "qcom/aic200/sdi.mbn",
> + [69] = "qcom/aic200/dcd.mbn",
> + [73] = "qcom/aic200/gearvm.mbn",
> + [74] = "qcom/aic200/sti.bin",
> + [75] = "qcom/aic200/pvs.bin",
> +};
> +
> +const size_t aic200_image_table_size = ARRAY_SIZE(aic200_image_table);
> +
> static int mhi_read_reg(struct mhi_controller *mhi_cntrl, void __iomem *addr, u32 *out)
> {
> u32 tmp;
> diff --git a/drivers/accel/qaic/sahara.c b/drivers/accel/qaic/sahara.c
> index 3ebcc1f7ff58..cf8f8b585223 100644
> --- a/drivers/accel/qaic/sahara.c
> +++ b/drivers/accel/qaic/sahara.c
> @@ -177,45 +177,6 @@ struct sahara_context {
> bool is_mem_dump_mode;
> };
>
> -static const char * const aic100_image_table[] = {
> - [1] = "qcom/aic100/fw1.bin",
> - [2] = "qcom/aic100/fw2.bin",
> - [4] = "qcom/aic100/fw4.bin",
> - [5] = "qcom/aic100/fw5.bin",
> - [6] = "qcom/aic100/fw6.bin",
> - [8] = "qcom/aic100/fw8.bin",
> - [9] = "qcom/aic100/fw9.bin",
> - [10] = "qcom/aic100/fw10.bin",
> -};
> -
> -static const char * const aic200_image_table[] = {
> - [5] = "qcom/aic200/uefi.elf",
> - [12] = "qcom/aic200/aic200-nsp.bin",
> - [23] = "qcom/aic200/aop.mbn",
> - [32] = "qcom/aic200/tz.mbn",
> - [33] = "qcom/aic200/hypvm.mbn",
> - [39] = "qcom/aic200/aic200_abl.elf",
> - [40] = "qcom/aic200/apdp.mbn",
> - [41] = "qcom/aic200/devcfg.mbn",
> - [42] = "qcom/aic200/sec.elf",
> - [43] = "qcom/aic200/aic200-hlos.elf",
> - [49] = "qcom/aic200/shrm.elf",
> - [50] = "qcom/aic200/cpucp.elf",
> - [51] = "qcom/aic200/aop_devcfg.mbn",
> - [57] = "qcom/aic200/cpucp_dtbs.elf",
> - [62] = "qcom/aic200/uefi_dtbs.elf",
> - [63] = "qcom/aic200/xbl_ac_config.mbn",
> - [64] = "qcom/aic200/tz_ac_config.mbn",
> - [65] = "qcom/aic200/hyp_ac_config.mbn",
> - [66] = "qcom/aic200/pdp.elf",
> - [67] = "qcom/aic200/pdp_cdb.elf",
> - [68] = "qcom/aic200/sdi.mbn",
> - [69] = "qcom/aic200/dcd.mbn",
> - [73] = "qcom/aic200/gearvm.mbn",
> - [74] = "qcom/aic200/sti.bin",
> - [75] = "qcom/aic200/pvs.bin",
> -};
> -
> static int sahara_find_image(struct sahara_context *context, u32 image_id)
> {
> int ret;
> @@ -779,10 +740,10 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
>
> if (!strcmp(mhi_dev->mhi_cntrl->name, "AIC200")) {
> context->image_table = aic200_image_table;
> - context->table_size = ARRAY_SIZE(aic200_image_table);
> + context->table_size = aic200_image_table_size;
> } else {
> context->image_table = aic100_image_table;
> - context->table_size = ARRAY_SIZE(aic100_image_table);
> + context->table_size = aic100_image_table_size;
> }
>
> context->active_image_id = SAHARA_IMAGE_ID_NONE;
> diff --git a/drivers/accel/qaic/sahara.h b/drivers/accel/qaic/sahara.h
> index 640208acc0d1..d7fd447ca85b 100644
> --- a/drivers/accel/qaic/sahara.h
> +++ b/drivers/accel/qaic/sahara.h
> @@ -7,4 +7,11 @@
>
> int sahara_register(void);
> void sahara_unregister(void);
> +
> +extern const char * const aic200_image_table[];
> +extern const size_t aic200_image_table_size;
> +
> +extern const char * const aic100_image_table[];
> +extern const size_t aic100_image_table_size;
Making sahara.c reference these arrays through extern declarations is
pretty ugly, and in patch 4 you're forgetting to add the "static"
keyword to the image_tables...
How about introducing patch 3 first, with special handling for AIC[12]00
and then squash this with what is now patch 4 to make the move in one
go?
Still need a statement in one of these commit messages to why you don't
just add qdu_image_table[] to sahara.c...
Regards,
Bjorn
> +
> #endif /* __SAHARA_H__ */
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 03/12] drivers: accel: qaic: Support for registration of image tables in Sahara.
2025-08-25 10:19 [PATCH v1 00/12] Sahara protocol enhancements Kishore Batta
2025-08-25 10:19 ` [PATCH v1 01/12] Add documentation for Sahara protocol Kishore Batta
2025-08-25 10:19 ` [PATCH v1 02/12] drivers: accel : Move AIC specific image tables to mhi_controller.c file Kishore Batta
@ 2025-08-25 10:19 ` Kishore Batta
2025-08-25 21:26 ` Bjorn Andersson
2025-08-25 10:19 ` [PATCH v1 04/12] drivers: accel: Register Qualcomm AIC specific image tables with Sahara Kishore Batta
` (8 subsequent siblings)
11 siblings, 1 reply; 26+ messages in thread
From: Kishore Batta @ 2025-08-25 10:19 UTC (permalink / raw)
To: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio
Cc: andersson, linux-arm-msm
Support the registration of image tables in the Sahara driver. Each
Qualcomm device can define its own image table, and client drivers can
register their image tables with the Sahara protocol. The Sahara protocol
driver now exposes the necessary APIs to facilitate image table
registration and de-registration. These image tables are used by Sahara
to transfer images from the host filesystem to the device.
Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
---
drivers/accel/qaic/Makefile | 3 +-
drivers/accel/qaic/sahara_image_table.c | 173 ++++++++++++++++++++
drivers/accel/qaic/sahara_image_table_ops.h | 102 ++++++++++++
3 files changed, 277 insertions(+), 1 deletion(-)
create mode 100644 drivers/accel/qaic/sahara_image_table.c
create mode 100644 drivers/accel/qaic/sahara_image_table_ops.h
diff --git a/drivers/accel/qaic/Makefile b/drivers/accel/qaic/Makefile
index 1106b876f737..586a6877e568 100644
--- a/drivers/accel/qaic/Makefile
+++ b/drivers/accel/qaic/Makefile
@@ -12,6 +12,7 @@ qaic-y := \
qaic_drv.o \
qaic_ras.o \
qaic_timesync.o \
- sahara.o
+ sahara.o \
+ sahara_image_table.o
qaic-$(CONFIG_DEBUG_FS) += qaic_debugfs.o
diff --git a/drivers/accel/qaic/sahara_image_table.c b/drivers/accel/qaic/sahara_image_table.c
new file mode 100644
index 000000000000..dd0793a33727
--- /dev/null
+++ b/drivers/accel/qaic/sahara_image_table.c
@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. */
+
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+
+#include "sahara_image_table_ops.h"
+
+struct sahara_image_table_context {
+ struct list_head provider_list;
+ /* Protects access to provider_list and related operations */
+ struct mutex provider_mutex;
+};
+
+static struct sahara_image_table_context sahara_img_ctx = {
+ .provider_list = LIST_HEAD_INIT(sahara_img_ctx.provider_list),
+ .provider_mutex = __MUTEX_INITIALIZER(sahara_img_ctx.provider_mutex),
+};
+
+/**
+ * sahara_register_image_table_provider - Register an image table provider.
+ * @provider: Pointer to the sahara_image_table_provider structure to be
+ * registered.
+ *
+ * This function validates the provided sahara_image_table_provider structure
+ * and adds it to the global list of image table providers. The list is
+ * protected by a mutex to ensure thread-safe operations.
+ *
+ * Return: 0 on success, -EINVAL if the provider or its required fields are
+ * invalid.
+ */
+int sahara_register_image_table_provider(struct sahara_image_table_provider
+ *provider)
+{
+ /* Validate the provider and its required fields */
+ if (!provider || !provider->image_table || !provider->dev_name)
+ return -EINVAL;
+
+ /* Acquire the mutex before modifying the list */
+ mutex_lock(&sahara_img_ctx.provider_mutex);
+
+ /* Add the provider to the list */
+ list_add(&provider->list, &sahara_img_ctx.provider_list);
+
+ /* Release the mutex after modification */
+ mutex_unlock(&sahara_img_ctx.provider_mutex);
+
+ return 0;
+}
+
+/**
+ * sahara_get_image_table - Get the image table for a given device name
+ * @dev_name: The name of the device for which the image table is requested.
+ *
+ * This function iterates through the list of registered image table providers
+ * and returns the image table for the provider matching the given device name.
+ *
+ * Return: A pointer to the image table if found, or NULL if no matching
+ * provider is found.
+ */
+const char * const *sahara_get_image_table(const char *dev_name)
+{
+ struct sahara_image_table_provider *provider;
+
+ /* Validate the device name */
+ if (!dev_name) {
+ pr_debug("sahara: Invalid argument %s\n", dev_name);
+ return NULL;
+ }
+
+ /* Iterate through the list to find the matching provider */
+ list_for_each_entry(provider, &sahara_img_ctx.provider_list, list) {
+ if (strcmp(provider->dev_name, dev_name) == 0)
+ return provider->image_table;
+ }
+
+ return NULL;
+}
+
+/**
+ * sahara_get_image_table_size - Get the size of the image table for a given
+ * device name.
+ * @dev_name: The name of the device for which the image table size is requested
+ *
+ * This function iterates through the list of registered image table providers
+ * and returns the size of the image table for the provider matching the given
+ * device name.
+ *
+ * Return: The size of the image table if found, or 0 if no matching provider
+ * is found or if the device name is invalid.
+ */
+size_t sahara_get_image_table_size(const char *dev_name)
+{
+ struct sahara_image_table_provider *provider;
+
+ /* Validate the dev name */
+ if (!dev_name) {
+ pr_debug("sahara: Invalid argument %s\n", dev_name);
+ return 0;
+ }
+
+ /* Iterate through the list to find the matching provider */
+ list_for_each_entry(provider, &sahara_img_ctx.provider_list, list) {
+ if (strcmp(provider->dev_name, dev_name) == 0)
+ return provider->image_table_size;
+ }
+
+ return 0;
+}
+
+/**
+ * sahara_unregister_image_table_provider - Unregister an image table provider.
+ * @provider: Pointer to the sahara_image_table_provider structure to be
+ * unregistered
+ *
+ * This function validates the provided sahara_image_table_provider structure
+ * and removes it from the global list of image table providers. The list is
+ * protected by a mutex to ensure thread-safe operations.
+ *
+ * Return: 0 on success, -EINVAL if the provider is invalid.
+ */
+int sahara_unregister_image_table_provider(struct sahara_image_table_provider
+ *provider)
+{
+ /* Validate the provider */
+ if (!provider)
+ return -EINVAL;
+
+ /* Acquire the mutex before modifying the list */
+ mutex_lock(&sahara_img_ctx.provider_mutex);
+
+ /* Remove the provider from the list */
+ list_del(&provider->list);
+
+ /* Release the mutex after modification */
+ mutex_unlock(&sahara_img_ctx.provider_mutex);
+
+ return 0;
+}
+
+/**
+ * sahara_get_fw_folder_name - Retrieve the firmware folder name for a given
+ * device
+ * @dev_name: Name of the device for which the firmware folder name is requested
+ *
+ * This function searches through the list of Sahara image table providers to
+ * find the provider matching the given device name. If a matching provider is
+ * found, the firmware folder name associated with that provider is returned.
+ * If the device name is invalid or no matching provider is found, the function
+ * returns NULL.
+ *
+ * Return: Firmware folder name if found, otherwise NULL.
+ */
+char *sahara_get_fw_folder_name(const char *dev_name)
+{
+ struct sahara_image_table_provider *provider;
+
+ /* Validate the device name */
+ if (!dev_name) {
+ pr_debug("sahara: Invalid argument %s\n", dev_name);
+ return NULL;
+ }
+
+ /* Iterate through the list to find the matching provider */
+ list_for_each_entry(provider, &sahara_img_ctx.provider_list, list) {
+ if (strcmp(provider->dev_name, dev_name) == 0)
+ return provider->fw_folder_name;
+ }
+
+ return NULL;
+}
diff --git a/drivers/accel/qaic/sahara_image_table_ops.h b/drivers/accel/qaic/sahara_image_table_ops.h
new file mode 100644
index 000000000000..f8496bd1aa35
--- /dev/null
+++ b/drivers/accel/qaic/sahara_image_table_ops.h
@@ -0,0 +1,102 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. */
+
+#ifndef __SAHARA_IMAGE_TABLE_OPS_H__
+#define __SAHARA_IMAGE_TABLE_OPS_H__
+
+#include <linux/list.h>
+
+/**
+ * struct sahara_image_table_provider - Structure representing an image table
+ * provider.
+ * @image_table: Pointer to the image table
+ * @image_table_size: Size of the image table
+ * @dev_name: Device name to identify the provider
+ * @fw_folder_name: Name of the folder where the image binaries exist.
+ * @list: List head for linking providers in a list
+ *
+ * This structure is used to represent an image table provider in the Sahara
+ * driver. It contains a pointer to the image table, the size of the image
+ * table, the device name for identifying the provider, and a list head for
+ * linking providers in a linked list.
+ */
+struct sahara_image_table_provider {
+ const char * const *image_table;
+ size_t image_table_size;
+ const char *dev_name;
+ char *fw_folder_name;
+ struct list_head list;
+};
+
+/**
+ * sahara_register_image_table_provider - Register an image table provider.
+ * @provider: Pointer to the sahara_image_table_provider structure to be
+ * registered.
+ *
+ * This function validates the provided sahara_image_table_provider structure
+ * and adds it to the global list of image table providers. The list is
+ * protected by a mutex to ensure thread-safe operations.
+ *
+ * Return: 0 on success, -EINVAL if the provider or its required fields are
+ * invalid.
+ */
+int sahara_register_image_table_provider(struct sahara_image_table_provider
+ *provider);
+
+/**
+ * sahara_get_image_table - Get the image table for a given device name
+ * @dev_name: The name of the device for which the image table is requested.
+ *
+ * This function iterates through the list of registered image table providers
+ * and returns the image table for the provider matching the given device name.
+ *
+ * Return: A pointer to the image table if found, or NULL if no matching
+ * provider is found.
+ */
+const char * const *sahara_get_image_table(const char *dev_name);
+
+/**
+ * sahara_get_image_table_size - Get the size of the image table for a given
+ * device name.
+ * @dev_name: The name of the device for which the image table size is requested
+ *
+ * This function iterates through the list of registered image table providers
+ * and returns the size of the image table for the provider matching the given
+ * device name.
+ *
+ * Return: The size of the image table if found, or 0 if no matching provider
+ * is found or if the device name is invalid.
+ */
+size_t sahara_get_image_table_size(const char *dev_name);
+
+/**
+ * sahara_unregister_image_table_provider - Unregister an image table provider.
+ * @provider: Pointer to the sahara_image_table_provider structure to be
+ * unregistered
+ *
+ * This function validates the provided sahara_image_table_provider structure
+ * and removes it from the global list of image table providers. The list is
+ * protected by a mutex to ensure thread-safe operations.
+ *
+ * Return: 0 on success, -EINVAL if the provider is invalid.
+ */
+int sahara_unregister_image_table_provider(struct sahara_image_table_provider
+ *provider);
+
+/**
+ * sahara_get_fw_folder_name - Retrieve the firmware folder name for a given
+ * device
+ * @dev_name: Name of the device for which the firmware folder name is requested
+ *
+ * This function searches through the list of Sahara image table providers to
+ * find the provider matching the given device name. If a matching provider is
+ * found, the firmware folder name associated with that provider is returned.
+ * If the device name is invalid or no matching provider is found, the function
+ * returns NULL.
+ *
+ * Return: Firmware folder name if found, otherwise NULL.
+ */
+char *sahara_get_fw_folder_name(const char *dev_name);
+
+#endif // __SAHARA_IMAGE_TABLE_OPS_H__
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v1 03/12] drivers: accel: qaic: Support for registration of image tables in Sahara.
2025-08-25 10:19 ` [PATCH v1 03/12] drivers: accel: qaic: Support for registration of image tables in Sahara Kishore Batta
@ 2025-08-25 21:26 ` Bjorn Andersson
0 siblings, 0 replies; 26+ messages in thread
From: Bjorn Andersson @ 2025-08-25 21:26 UTC (permalink / raw)
To: Kishore Batta
Cc: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio,
linux-arm-msm
On Mon, Aug 25, 2025 at 03:49:17PM +0530, Kishore Batta wrote:
Just noticed that your recipients list doesn't match get_maintainers.
Please adopt b4 and let it choose recipients for you.
And same subject prefix issues as all the patches.
> Support the registration of image tables in the Sahara driver. Each
> Qualcomm device can define its own image table, and client drivers can
> register their image tables with the Sahara protocol. The Sahara protocol
> driver now exposes the necessary APIs to facilitate image table
> registration and de-registration. These image tables are used by Sahara
> to transfer images from the host filesystem to the device.
>
> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
> ---
> drivers/accel/qaic/Makefile | 3 +-
> drivers/accel/qaic/sahara_image_table.c | 173 ++++++++++++++++++++
> drivers/accel/qaic/sahara_image_table_ops.h | 102 ++++++++++++
> 3 files changed, 277 insertions(+), 1 deletion(-)
> create mode 100644 drivers/accel/qaic/sahara_image_table.c
> create mode 100644 drivers/accel/qaic/sahara_image_table_ops.h
>
> diff --git a/drivers/accel/qaic/Makefile b/drivers/accel/qaic/Makefile
> index 1106b876f737..586a6877e568 100644
> --- a/drivers/accel/qaic/Makefile
> +++ b/drivers/accel/qaic/Makefile
> @@ -12,6 +12,7 @@ qaic-y := \
> qaic_drv.o \
> qaic_ras.o \
> qaic_timesync.o \
> - sahara.o
> + sahara.o \
> + sahara_image_table.o
>
> qaic-$(CONFIG_DEBUG_FS) += qaic_debugfs.o
> diff --git a/drivers/accel/qaic/sahara_image_table.c b/drivers/accel/qaic/sahara_image_table.c
> new file mode 100644
> index 000000000000..dd0793a33727
> --- /dev/null
> +++ b/drivers/accel/qaic/sahara_image_table.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. */
> +
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +
> +#include "sahara_image_table_ops.h"
> +
> +struct sahara_image_table_context {
> + struct list_head provider_list;
> + /* Protects access to provider_list and related operations */
> + struct mutex provider_mutex;
> +};
Drop this struct and turn the two members global variables.
> +
> +static struct sahara_image_table_context sahara_img_ctx = {
> + .provider_list = LIST_HEAD_INIT(sahara_img_ctx.provider_list),
> + .provider_mutex = __MUTEX_INITIALIZER(sahara_img_ctx.provider_mutex),
> +};
> +
> +/**
> + * sahara_register_image_table_provider - Register an image table provider.
https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
says you should put () after the function name.
> + * @provider: Pointer to the sahara_image_table_provider structure to be
> + * registered.
> + *
> + * This function validates the provided sahara_image_table_provider structure
> + * and adds it to the global list of image table providers.
What is the key thing this function does? It validates the
image_table_provider! And then second to that it might add it to some
list...
> The list is
> + * protected by a mutex to ensure thread-safe operations.
https://docs.kernel.org/doc-guide/kernel-doc.html#function-context
> + *
> + * Return: 0 on success, -EINVAL if the provider or its required fields are
> + * invalid.
> + */
> +int sahara_register_image_table_provider(struct sahara_image_table_provider
> + *provider)
> +{
> + /* Validate the provider and its required fields */
> + if (!provider || !provider->image_table || !provider->dev_name)
> + return -EINVAL;
> +
> + /* Acquire the mutex before modifying the list */
If that isn't obvious from the line mutex_lock(something) consider
giving the lock a better name.
It's obvious what this sequence does
lock()
modify(list)
unlock()
Document things that aren't obvious.
> + mutex_lock(&sahara_img_ctx.provider_mutex);
> +
> + /* Add the provider to the list */
> + list_add(&provider->list, &sahara_img_ctx.provider_list);
> +
> + /* Release the mutex after modification */
> + mutex_unlock(&sahara_img_ctx.provider_mutex);
> +
> + return 0;
> +}
> +
> +/**
> + * sahara_get_image_table - Get the image table for a given device name
> + * @dev_name: The name of the device for which the image table is requested.
> + *
> + * This function iterates through the list of registered image table providers
> + * and returns the image table for the provider matching the given device name.
> + *
> + * Return: A pointer to the image table if found, or NULL if no matching
> + * provider is found.
> + */
> +const char * const *sahara_get_image_table(const char *dev_name)
> +{
> + struct sahara_image_table_provider *provider;
> +
> + /* Validate the device name */
> + if (!dev_name) {
> + pr_debug("sahara: Invalid argument %s\n", dev_name);
> + return NULL;
> + }
This is overly defensive. You're writing the only code that should ever
call this function, just make sure you don't explicitly pass NULL when
you do...
> +
> + /* Iterate through the list to find the matching provider */
> + list_for_each_entry(provider, &sahara_img_ctx.provider_list, list) {
> + if (strcmp(provider->dev_name, dev_name) == 0)
> + return provider->image_table;
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * sahara_get_image_table_size - Get the size of the image table for a given
> + * device name.
> + * @dev_name: The name of the device for which the image table size is requested
> + *
> + * This function iterates through the list of registered image table providers
> + * and returns the size of the image table for the provider matching the given
> + * device name.
> + *
> + * Return: The size of the image table if found, or 0 if no matching provider
> + * is found or if the device name is invalid.
> + */
> +size_t sahara_get_image_table_size(const char *dev_name)
You don't need two identical functions for getting the table and its
size, just add a "size_t *size" parameter to sahara_get_image_table()
and return both values in one - saves you 29 lines of ~copy-pasta.
> +{
> + struct sahara_image_table_provider *provider;
> +
> + /* Validate the dev name */
> + if (!dev_name) {
> + pr_debug("sahara: Invalid argument %s\n", dev_name);
> + return 0;
> + }
> +
> + /* Iterate through the list to find the matching provider */
> + list_for_each_entry(provider, &sahara_img_ctx.provider_list, list) {
> + if (strcmp(provider->dev_name, dev_name) == 0)
> + return provider->image_table_size;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * sahara_unregister_image_table_provider - Unregister an image table provider.
> + * @provider: Pointer to the sahara_image_table_provider structure to be
> + * unregistered
> + *
> + * This function validates the provided sahara_image_table_provider structure
> + * and removes it from the global list of image table providers. The list is
> + * protected by a mutex to ensure thread-safe operations.
> + *
> + * Return: 0 on success, -EINVAL if the provider is invalid.
> + */
> +int sahara_unregister_image_table_provider(struct sahara_image_table_provider
> + *provider)
unregister functions typically return void, because there isn't much
useful one can do if it fails.
> +{
> + /* Validate the provider */
> + if (!provider)
> + return -EINVAL;
This doesn't really check that the point is valid, just that it's not
NULL. And per the intended usage, that can never happen. So I'd suggest
dropping this check.
> +
> + /* Acquire the mutex before modifying the list */
> + mutex_lock(&sahara_img_ctx.provider_mutex);
> +
> + /* Remove the provider from the list */
> + list_del(&provider->list);
> +
> + /* Release the mutex after modification */
> + mutex_unlock(&sahara_img_ctx.provider_mutex);
> +
> + return 0;
> +}
> +
> +/**
> + * sahara_get_fw_folder_name - Retrieve the firmware folder name for a given
> + * device
> + * @dev_name: Name of the device for which the firmware folder name is requested
> + *
> + * This function searches through the list of Sahara image table providers to
> + * find the provider matching the given device name. If a matching provider is
> + * found, the firmware folder name associated with that provider is returned.
> + * If the device name is invalid or no matching provider is found, the function
> + * returns NULL.
> + *
> + * Return: Firmware folder name if found, otherwise NULL.
> + */
> +char *sahara_get_fw_folder_name(const char *dev_name)
> +{
> + struct sahara_image_table_provider *provider;
> +
> + /* Validate the device name */
> + if (!dev_name) {
> + pr_debug("sahara: Invalid argument %s\n", dev_name);
> + return NULL;
> + }
> +
> + /* Iterate through the list to find the matching provider */
> + list_for_each_entry(provider, &sahara_img_ctx.provider_list, list) {
> + if (strcmp(provider->dev_name, dev_name) == 0)
> + return provider->fw_folder_name;
> + }
> +
> + return NULL;
> +}
> diff --git a/drivers/accel/qaic/sahara_image_table_ops.h b/drivers/accel/qaic/sahara_image_table_ops.h
> new file mode 100644
> index 000000000000..f8496bd1aa35
> --- /dev/null
> +++ b/drivers/accel/qaic/sahara_image_table_ops.h
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +/* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. */
> +
> +#ifndef __SAHARA_IMAGE_TABLE_OPS_H__
> +#define __SAHARA_IMAGE_TABLE_OPS_H__
> +
> +#include <linux/list.h>
> +
> +/**
> + * struct sahara_image_table_provider - Structure representing an image table
> + * provider.
> + * @image_table: Pointer to the image table
> + * @image_table_size: Size of the image table
> + * @dev_name: Device name to identify the provider
> + * @fw_folder_name: Name of the folder where the image binaries exist.
> + * @list: List head for linking providers in a list
> + *
> + * This structure is used to represent an image table provider in the Sahara
> + * driver. It contains a pointer to the image table, the size of the image
> + * table, the device name for identifying the provider, and a list head for
> + * linking providers in a linked list.
> + */
> +struct sahara_image_table_provider {
> + const char * const *image_table;
> + size_t image_table_size;
> + const char *dev_name;
> + char *fw_folder_name;
> + struct list_head list;
> +};
> +
> +/**
> + * sahara_register_image_table_provider - Register an image table provider.
You already provide kernel-doc in the implementation, no need to
duplicate it also in the header file.
Regards,
Bjorn
> + * @provider: Pointer to the sahara_image_table_provider structure to be
> + * registered.
> + *
> + * This function validates the provided sahara_image_table_provider structure
> + * and adds it to the global list of image table providers. The list is
> + * protected by a mutex to ensure thread-safe operations.
> + *
> + * Return: 0 on success, -EINVAL if the provider or its required fields are
> + * invalid.
> + */
> +int sahara_register_image_table_provider(struct sahara_image_table_provider
> + *provider);
> +
> +/**
> + * sahara_get_image_table - Get the image table for a given device name
> + * @dev_name: The name of the device for which the image table is requested.
> + *
> + * This function iterates through the list of registered image table providers
> + * and returns the image table for the provider matching the given device name.
> + *
> + * Return: A pointer to the image table if found, or NULL if no matching
> + * provider is found.
> + */
> +const char * const *sahara_get_image_table(const char *dev_name);
> +
> +/**
> + * sahara_get_image_table_size - Get the size of the image table for a given
> + * device name.
> + * @dev_name: The name of the device for which the image table size is requested
> + *
> + * This function iterates through the list of registered image table providers
> + * and returns the size of the image table for the provider matching the given
> + * device name.
> + *
> + * Return: The size of the image table if found, or 0 if no matching provider
> + * is found or if the device name is invalid.
> + */
> +size_t sahara_get_image_table_size(const char *dev_name);
> +
> +/**
> + * sahara_unregister_image_table_provider - Unregister an image table provider.
> + * @provider: Pointer to the sahara_image_table_provider structure to be
> + * unregistered
> + *
> + * This function validates the provided sahara_image_table_provider structure
> + * and removes it from the global list of image table providers. The list is
> + * protected by a mutex to ensure thread-safe operations.
> + *
> + * Return: 0 on success, -EINVAL if the provider is invalid.
> + */
> +int sahara_unregister_image_table_provider(struct sahara_image_table_provider
> + *provider);
> +
> +/**
> + * sahara_get_fw_folder_name - Retrieve the firmware folder name for a given
> + * device
> + * @dev_name: Name of the device for which the firmware folder name is requested
> + *
> + * This function searches through the list of Sahara image table providers to
> + * find the provider matching the given device name. If a matching provider is
> + * found, the firmware folder name associated with that provider is returned.
> + * If the device name is invalid or no matching provider is found, the function
> + * returns NULL.
> + *
> + * Return: Firmware folder name if found, otherwise NULL.
> + */
> +char *sahara_get_fw_folder_name(const char *dev_name);
> +
> +#endif // __SAHARA_IMAGE_TABLE_OPS_H__
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 04/12] drivers: accel: Register Qualcomm AIC specific image tables with Sahara.
2025-08-25 10:19 [PATCH v1 00/12] Sahara protocol enhancements Kishore Batta
` (2 preceding siblings ...)
2025-08-25 10:19 ` [PATCH v1 03/12] drivers: accel: qaic: Support for registration of image tables in Sahara Kishore Batta
@ 2025-08-25 10:19 ` Kishore Batta
2025-08-25 21:38 ` Bjorn Andersson
2025-08-25 10:19 ` [PATCH v1 05/12] drivers: soc: qcom: Move Sahara driver to drivers/soc/qcom directory Kishore Batta
` (7 subsequent siblings)
11 siblings, 1 reply; 26+ messages in thread
From: Kishore Batta @ 2025-08-25 10:19 UTC (permalink / raw)
To: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio
Cc: andersson, linux-arm-msm
Register Qualcomm AIC-specific image tables with the Sahara protocol.
The Sahara protocol provides a method for client drivers to register
device-specific image tables, which is mandatory for firmware transfer.
During QAIC device initialization, the QAIC driver must register the
image table information with the Sahara protocol for firmware transfer
to occur. Once the device is probed, it sends the required Sahara packets
to the host. Based on the connected device, Sahara selects the appropriate
image table and sends the firmware image data back to the device.
Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
---
drivers/accel/qaic/mhi_controller.c | 57 +++++++++++++++++++++++++++--
drivers/accel/qaic/mhi_controller.h | 2 +
drivers/accel/qaic/qaic_drv.c | 7 ++++
drivers/accel/qaic/sahara.c | 17 +++++----
drivers/accel/qaic/sahara.h | 6 ---
5 files changed, 73 insertions(+), 16 deletions(-)
diff --git a/drivers/accel/qaic/mhi_controller.c b/drivers/accel/qaic/mhi_controller.c
index 5cc7994f4809..16c346e0e3b5 100644
--- a/drivers/accel/qaic/mhi_controller.c
+++ b/drivers/accel/qaic/mhi_controller.c
@@ -13,6 +13,7 @@
#include "mhi_controller.h"
#include "qaic.h"
+#include "sahara_image_table_ops.h"
#define MAX_RESET_TIME_SEC 25
@@ -801,8 +802,6 @@ const char * const aic100_image_table[] = {
[10] = "qcom/aic100/fw10.bin",
};
-const size_t aic100_image_table_size = ARRAY_SIZE(aic100_image_table);
-
const char * const aic200_image_table[] = {
[5] = "qcom/aic200/uefi.elf",
[12] = "qcom/aic200/aic200-nsp.bin",
@@ -831,7 +830,59 @@ const char * const aic200_image_table[] = {
[75] = "qcom/aic200/pvs.bin",
};
-const size_t aic200_image_table_size = ARRAY_SIZE(aic200_image_table);
+static struct sahara_image_table_provider aic100_provider = {
+ .image_table = aic100_image_table,
+ .image_table_size = ARRAY_SIZE(aic100_image_table),
+ .dev_name = "AIC100",
+ .fw_folder_name = "aic100",
+ .list = LIST_HEAD_INIT(aic100_provider.list)
+};
+
+static struct sahara_image_table_provider aic200_provider = {
+ .image_table = aic200_image_table,
+ .image_table_size = ARRAY_SIZE(aic200_image_table),
+ .dev_name = "AIC200",
+ .fw_folder_name = "aic200",
+ .list = LIST_HEAD_INIT(aic200_provider.list)
+};
+
+static struct sahara_image_table_provider *aic_providers[] = {
+ &aic100_provider,
+ &aic200_provider,
+};
+
+int qaic_sahara_register_image_tables(void)
+{
+ int ret;
+
+ for (int i = 0; i < ARRAY_SIZE(aic_providers); i++) {
+ ret = sahara_register_image_table_provider(aic_providers[i]);
+ if (ret) {
+ pr_err("qaic: Failed to register image table %d\n",
+ ret);
+
+ /* Rollback previously registered providers */
+ while (--i >= 0)
+ sahara_unregister_image_table_provider(aic_providers[i]);
+
+ return ret;
+ }
+ }
+ return 0;
+}
+
+void qaic_sahara_unregister_image_tables(void)
+{
+ int ret;
+
+ for (int i = 0; i < ARRAY_SIZE(aic_providers); i++) {
+ ret = sahara_unregister_image_table_provider(aic_providers[i]);
+ if (ret)
+ pr_err("qaic: Failed to unregister image table %d\n",
+ ret);
+ }
+}
+
static int mhi_read_reg(struct mhi_controller *mhi_cntrl, void __iomem *addr, u32 *out)
{
diff --git a/drivers/accel/qaic/mhi_controller.h b/drivers/accel/qaic/mhi_controller.h
index 8939f6ae185e..90c0f07cbdf6 100644
--- a/drivers/accel/qaic/mhi_controller.h
+++ b/drivers/accel/qaic/mhi_controller.h
@@ -12,5 +12,7 @@ struct mhi_controller *qaic_mhi_register_controller(struct pci_dev *pci_dev, voi
void qaic_mhi_free_controller(struct mhi_controller *mhi_cntrl, bool link_up);
void qaic_mhi_start_reset(struct mhi_controller *mhi_cntrl);
void qaic_mhi_reset_done(struct mhi_controller *mhi_cntrl);
+int qaic_sahara_register_image_tables(void);
+void qaic_sahara_unregister_image_tables(void);
#endif /* MHICONTROLLERQAIC_H_ */
diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
index e31bcb0ecfc9..5c4fab328003 100644
--- a/drivers/accel/qaic/qaic_drv.c
+++ b/drivers/accel/qaic/qaic_drv.c
@@ -688,6 +688,12 @@ static int __init qaic_init(void)
goto free_mhi;
}
+ ret = qaic_sahara_register_image_tables();
+ if (ret) {
+ pr_debug("qaic: Image table registration failed %d\n", ret);
+ goto free_mhi;
+ }
+
ret = qaic_timesync_init();
if (ret)
pr_debug("qaic: qaic_timesync_init failed %d\n", ret);
@@ -727,6 +733,7 @@ static void __exit qaic_exit(void)
* reinitializing the link_up state after the cleanup is done.
*/
link_up = true;
+ qaic_sahara_unregister_image_tables();
qaic_ras_unregister();
qaic_bootlog_unregister();
qaic_timesync_deinit();
diff --git a/drivers/accel/qaic/sahara.c b/drivers/accel/qaic/sahara.c
index cf8f8b585223..7eae329396be 100644
--- a/drivers/accel/qaic/sahara.c
+++ b/drivers/accel/qaic/sahara.c
@@ -14,6 +14,7 @@
#include <linux/workqueue.h>
#include "sahara.h"
+#include "sahara_image_table_ops.h"
#define SAHARA_HELLO_CMD 0x1 /* Min protocol version 1.0 */
#define SAHARA_HELLO_RESP_CMD 0x2 /* Min protocol version 1.0 */
@@ -738,13 +739,15 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
INIT_WORK(&context->fw_work, sahara_processing);
INIT_WORK(&context->dump_work, sahara_dump_processing);
- if (!strcmp(mhi_dev->mhi_cntrl->name, "AIC200")) {
- context->image_table = aic200_image_table;
- context->table_size = aic200_image_table_size;
- } else {
- context->image_table = aic100_image_table;
- context->table_size = aic100_image_table_size;
- }
+ /* Get the image table for a given device name */
+ context->image_table = sahara_get_image_table(mhi_dev->mhi_cntrl->name);
+ if (!context->image_table)
+ return -EINVAL;
+
+ /* Get the image table size for a given device name */
+ context->table_size = sahara_get_image_table_size(mhi_dev->mhi_cntrl->name);
+ if (!context->table_size)
+ return -EINVAL;
context->active_image_id = SAHARA_IMAGE_ID_NONE;
dev_set_drvdata(&mhi_dev->dev, context);
diff --git a/drivers/accel/qaic/sahara.h b/drivers/accel/qaic/sahara.h
index d7fd447ca85b..dde8c736d29e 100644
--- a/drivers/accel/qaic/sahara.h
+++ b/drivers/accel/qaic/sahara.h
@@ -8,10 +8,4 @@
int sahara_register(void);
void sahara_unregister(void);
-extern const char * const aic200_image_table[];
-extern const size_t aic200_image_table_size;
-
-extern const char * const aic100_image_table[];
-extern const size_t aic100_image_table_size;
-
#endif /* __SAHARA_H__ */
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v1 04/12] drivers: accel: Register Qualcomm AIC specific image tables with Sahara.
2025-08-25 10:19 ` [PATCH v1 04/12] drivers: accel: Register Qualcomm AIC specific image tables with Sahara Kishore Batta
@ 2025-08-25 21:38 ` Bjorn Andersson
0 siblings, 0 replies; 26+ messages in thread
From: Bjorn Andersson @ 2025-08-25 21:38 UTC (permalink / raw)
To: Kishore Batta
Cc: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio,
linux-arm-msm
On Mon, Aug 25, 2025 at 03:49:18PM +0530, Kishore Batta wrote:
> Register Qualcomm AIC-specific image tables with the Sahara protocol.
> The Sahara protocol provides a method for client drivers to register
> device-specific image tables, which is mandatory for firmware transfer.
> During QAIC device initialization, the QAIC driver must register the
> image table information with the Sahara protocol for firmware transfer
> to occur. Once the device is probed, it sends the required Sahara packets
> to the host. Based on the connected device, Sahara selects the appropriate
> image table and sends the firmware image data back to the device.
This does describe things that is happening. But it doesn't describe the
purpose of this patch.
>
> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
> ---
> drivers/accel/qaic/mhi_controller.c | 57 +++++++++++++++++++++++++++--
> drivers/accel/qaic/mhi_controller.h | 2 +
> drivers/accel/qaic/qaic_drv.c | 7 ++++
> drivers/accel/qaic/sahara.c | 17 +++++----
> drivers/accel/qaic/sahara.h | 6 ---
> 5 files changed, 73 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/accel/qaic/mhi_controller.c b/drivers/accel/qaic/mhi_controller.c
> index 5cc7994f4809..16c346e0e3b5 100644
> --- a/drivers/accel/qaic/mhi_controller.c
> +++ b/drivers/accel/qaic/mhi_controller.c
> @@ -13,6 +13,7 @@
>
> #include "mhi_controller.h"
> #include "qaic.h"
> +#include "sahara_image_table_ops.h"
>
> #define MAX_RESET_TIME_SEC 25
>
> @@ -801,8 +802,6 @@ const char * const aic100_image_table[] = {
> [10] = "qcom/aic100/fw10.bin",
> };
>
> -const size_t aic100_image_table_size = ARRAY_SIZE(aic100_image_table);
> -
> const char * const aic200_image_table[] = {
> [5] = "qcom/aic200/uefi.elf",
> [12] = "qcom/aic200/aic200-nsp.bin",
> @@ -831,7 +830,59 @@ const char * const aic200_image_table[] = {
> [75] = "qcom/aic200/pvs.bin",
> };
>
> -const size_t aic200_image_table_size = ARRAY_SIZE(aic200_image_table);
> +static struct sahara_image_table_provider aic100_provider = {
> + .image_table = aic100_image_table,
> + .image_table_size = ARRAY_SIZE(aic100_image_table),
> + .dev_name = "AIC100",
> + .fw_folder_name = "aic100",
> + .list = LIST_HEAD_INIT(aic100_provider.list)
> +};
> +
> +static struct sahara_image_table_provider aic200_provider = {
> + .image_table = aic200_image_table,
> + .image_table_size = ARRAY_SIZE(aic200_image_table),
> + .dev_name = "AIC200",
> + .fw_folder_name = "aic200",
> + .list = LIST_HEAD_INIT(aic200_provider.list)
> +};
> +
> +static struct sahara_image_table_provider *aic_providers[] = {
> + &aic100_provider,
> + &aic200_provider,
> +};
> +
> +int qaic_sahara_register_image_tables(void)
> +{
> + int ret;
> +
> + for (int i = 0; i < ARRAY_SIZE(aic_providers); i++) {
> + ret = sahara_register_image_table_provider(aic_providers[i]);
> + if (ret) {
> + pr_err("qaic: Failed to register image table %d\n",
> + ret);
> +
> + /* Rollback previously registered providers */
> + while (--i >= 0)
> + sahara_unregister_image_table_provider(aic_providers[i]);
> +
> + return ret;
> + }
> + }
> + return 0;
> +}
> +
> +void qaic_sahara_unregister_image_tables(void)
> +{
> + int ret;
> +
> + for (int i = 0; i < ARRAY_SIZE(aic_providers); i++) {
> + ret = sahara_unregister_image_table_provider(aic_providers[i]);
> + if (ret)
> + pr_err("qaic: Failed to unregister image table %d\n",
> + ret);
> + }
> +}
> +
>
> static int mhi_read_reg(struct mhi_controller *mhi_cntrl, void __iomem *addr, u32 *out)
> {
> diff --git a/drivers/accel/qaic/mhi_controller.h b/drivers/accel/qaic/mhi_controller.h
> index 8939f6ae185e..90c0f07cbdf6 100644
> --- a/drivers/accel/qaic/mhi_controller.h
> +++ b/drivers/accel/qaic/mhi_controller.h
> @@ -12,5 +12,7 @@ struct mhi_controller *qaic_mhi_register_controller(struct pci_dev *pci_dev, voi
> void qaic_mhi_free_controller(struct mhi_controller *mhi_cntrl, bool link_up);
> void qaic_mhi_start_reset(struct mhi_controller *mhi_cntrl);
> void qaic_mhi_reset_done(struct mhi_controller *mhi_cntrl);
> +int qaic_sahara_register_image_tables(void);
> +void qaic_sahara_unregister_image_tables(void);
>
> #endif /* MHICONTROLLERQAIC_H_ */
> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index e31bcb0ecfc9..5c4fab328003 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
> @@ -688,6 +688,12 @@ static int __init qaic_init(void)
> goto free_mhi;
> }
>
> + ret = qaic_sahara_register_image_tables();
Now that you're doing this on a per-device basis (but actually per
driver), could this somehow be done from qaic_mhi_register_controller()
instead. So we don't run this code unless you actually have a QAIC
attached?
> + if (ret) {
> + pr_debug("qaic: Image table registration failed %d\n", ret);
That's not a debug print...which is also the reason why you pr_err()
inside the function. I.e. this is at best spamming the log.
> + goto free_mhi;
> + }
> +
Regards,
Bjorn
> ret = qaic_timesync_init();
> if (ret)
> pr_debug("qaic: qaic_timesync_init failed %d\n", ret);
> @@ -727,6 +733,7 @@ static void __exit qaic_exit(void)
> * reinitializing the link_up state after the cleanup is done.
> */
> link_up = true;
> + qaic_sahara_unregister_image_tables();
> qaic_ras_unregister();
> qaic_bootlog_unregister();
> qaic_timesync_deinit();
> diff --git a/drivers/accel/qaic/sahara.c b/drivers/accel/qaic/sahara.c
> index cf8f8b585223..7eae329396be 100644
> --- a/drivers/accel/qaic/sahara.c
> +++ b/drivers/accel/qaic/sahara.c
> @@ -14,6 +14,7 @@
> #include <linux/workqueue.h>
>
> #include "sahara.h"
> +#include "sahara_image_table_ops.h"
>
> #define SAHARA_HELLO_CMD 0x1 /* Min protocol version 1.0 */
> #define SAHARA_HELLO_RESP_CMD 0x2 /* Min protocol version 1.0 */
> @@ -738,13 +739,15 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
> INIT_WORK(&context->fw_work, sahara_processing);
> INIT_WORK(&context->dump_work, sahara_dump_processing);
>
> - if (!strcmp(mhi_dev->mhi_cntrl->name, "AIC200")) {
> - context->image_table = aic200_image_table;
> - context->table_size = aic200_image_table_size;
> - } else {
> - context->image_table = aic100_image_table;
> - context->table_size = aic100_image_table_size;
> - }
> + /* Get the image table for a given device name */
> + context->image_table = sahara_get_image_table(mhi_dev->mhi_cntrl->name);
> + if (!context->image_table)
> + return -EINVAL;
> +
> + /* Get the image table size for a given device name */
> + context->table_size = sahara_get_image_table_size(mhi_dev->mhi_cntrl->name);
> + if (!context->table_size)
> + return -EINVAL;
>
> context->active_image_id = SAHARA_IMAGE_ID_NONE;
> dev_set_drvdata(&mhi_dev->dev, context);
> diff --git a/drivers/accel/qaic/sahara.h b/drivers/accel/qaic/sahara.h
> index d7fd447ca85b..dde8c736d29e 100644
> --- a/drivers/accel/qaic/sahara.h
> +++ b/drivers/accel/qaic/sahara.h
> @@ -8,10 +8,4 @@
> int sahara_register(void);
> void sahara_unregister(void);
>
> -extern const char * const aic200_image_table[];
> -extern const size_t aic200_image_table_size;
> -
> -extern const char * const aic100_image_table[];
> -extern const size_t aic100_image_table_size;
> -
> #endif /* __SAHARA_H__ */
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 05/12] drivers: soc: qcom: Move Sahara driver to drivers/soc/qcom directory.
2025-08-25 10:19 [PATCH v1 00/12] Sahara protocol enhancements Kishore Batta
` (3 preceding siblings ...)
2025-08-25 10:19 ` [PATCH v1 04/12] drivers: accel: Register Qualcomm AIC specific image tables with Sahara Kishore Batta
@ 2025-08-25 10:19 ` Kishore Batta
2025-08-25 22:12 ` Bjorn Andersson
2025-08-25 10:19 ` [PATCH v1 06/12] drivers: soc: qcom: Add support for Qualcomm QDU device Kishore Batta
` (6 subsequent siblings)
11 siblings, 1 reply; 26+ messages in thread
From: Kishore Batta @ 2025-08-25 10:19 UTC (permalink / raw)
To: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio
Cc: andersson, linux-arm-msm
Move the Sahara protocol driver from the "drivers/accel" directory
to the "drivers/soc/qcom" directory. This change makes the Sahara
driver applicable to all Qualcomm devices, not just Qualcomm Accelerator
devices. It also facilitates adding support for new devices. Client drivers
can use the registration and deregistration functionalities of the Sahara
driver as needed.
Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
---
drivers/accel/qaic/Kconfig | 1 +
drivers/accel/qaic/Makefile | 4 +---
drivers/accel/qaic/mhi_controller.c | 2 +-
drivers/accel/qaic/qaic_drv.c | 9 +--------
drivers/soc/qcom/Kconfig | 6 ++++--
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/sahara/Kconfig | 17 +++++++++++++++++
drivers/soc/qcom/sahara/Makefile | 6 ++++++
.../{accel/qaic => soc/qcom/sahara}/sahara.c | 11 ++++++++---
.../qcom/sahara}/sahara_image_table.c | 7 ++++++-
{drivers/accel/qaic => include/linux}/sahara.h | 0
.../linux}/sahara_image_table_ops.h | 0
12 files changed, 46 insertions(+), 18 deletions(-)
create mode 100644 drivers/soc/qcom/sahara/Kconfig
create mode 100644 drivers/soc/qcom/sahara/Makefile
rename drivers/{accel/qaic => soc/qcom/sahara}/sahara.c (99%)
rename drivers/{accel/qaic => soc/qcom/sahara}/sahara_image_table.c (94%)
rename {drivers/accel/qaic => include/linux}/sahara.h (100%)
rename {drivers/accel/qaic => include/linux}/sahara_image_table_ops.h (100%)
diff --git a/drivers/accel/qaic/Kconfig b/drivers/accel/qaic/Kconfig
index 5e405a19c157..5e2ac1ecede3 100644
--- a/drivers/accel/qaic/Kconfig
+++ b/drivers/accel/qaic/Kconfig
@@ -8,6 +8,7 @@ config DRM_ACCEL_QAIC
depends on DRM_ACCEL
depends on PCI && HAS_IOMEM
depends on MHI_BUS
+ select QCOM_SAHARA_PROTOCOL
select CRC32
help
Enables driver for Qualcomm's Cloud AI accelerator PCIe cards that are
diff --git a/drivers/accel/qaic/Makefile b/drivers/accel/qaic/Makefile
index 586a6877e568..4ad84f7e2162 100644
--- a/drivers/accel/qaic/Makefile
+++ b/drivers/accel/qaic/Makefile
@@ -11,8 +11,6 @@ qaic-y := \
qaic_data.o \
qaic_drv.o \
qaic_ras.o \
- qaic_timesync.o \
- sahara.o \
- sahara_image_table.o
+ qaic_timesync.o
qaic-$(CONFIG_DEBUG_FS) += qaic_debugfs.o
diff --git a/drivers/accel/qaic/mhi_controller.c b/drivers/accel/qaic/mhi_controller.c
index 16c346e0e3b5..76beef6018a7 100644
--- a/drivers/accel/qaic/mhi_controller.c
+++ b/drivers/accel/qaic/mhi_controller.c
@@ -9,11 +9,11 @@
#include <linux/mhi.h>
#include <linux/moduleparam.h>
#include <linux/pci.h>
+#include <linux/sahara_image_table_ops.h>
#include <linux/sizes.h>
#include "mhi_controller.h"
#include "qaic.h"
-#include "sahara_image_table_ops.h"
#define MAX_RESET_TIME_SEC 25
diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
index 5c4fab328003..a55e279411c3 100644
--- a/drivers/accel/qaic/qaic_drv.c
+++ b/drivers/accel/qaic/qaic_drv.c
@@ -15,6 +15,7 @@
#include <linux/msi.h>
#include <linux/mutex.h>
#include <linux/pci.h>
+#include <linux/sahara.h>
#include <linux/spinlock.h>
#include <linux/workqueue.h>
#include <linux/wait.h>
@@ -31,7 +32,6 @@
#include "qaic_debugfs.h"
#include "qaic_ras.h"
#include "qaic_timesync.h"
-#include "sahara.h"
MODULE_IMPORT_NS("DMA_BUF");
@@ -682,12 +682,6 @@ static int __init qaic_init(void)
goto free_pci;
}
- ret = sahara_register();
- if (ret) {
- pr_debug("qaic: sahara_register failed %d\n", ret);
- goto free_mhi;
- }
-
ret = qaic_sahara_register_image_tables();
if (ret) {
pr_debug("qaic: Image table registration failed %d\n", ret);
@@ -737,7 +731,6 @@ static void __exit qaic_exit(void)
qaic_ras_unregister();
qaic_bootlog_unregister();
qaic_timesync_deinit();
- sahara_unregister();
mhi_driver_unregister(&qaic_mhi_driver);
pci_unregister_driver(&qaic_pci_driver);
}
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 2caadbbcf830..7ea4cff9a679 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -295,8 +295,6 @@ config QCOM_PBS
This module provides the APIs to the client drivers that wants to send the
PBS trigger event to the PBS RAM.
-endmenu
-
config QCOM_UBWC_CONFIG
tristate
help
@@ -304,3 +302,7 @@ config QCOM_UBWC_CONFIG
(UBWC) engines across various IP blocks, which need to be initialized
with coherent configuration data. This module functions as a single
source of truth for that information.
+
+source "drivers/soc/qcom/sahara/Kconfig"
+
+endmenu
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index b7f1d2a57367..99e490e3174e 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -40,3 +40,4 @@ qcom_ice-objs += ice.o
obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
obj-$(CONFIG_QCOM_PBS) += qcom-pbs.o
obj-$(CONFIG_QCOM_UBWC_CONFIG) += ubwc_config.o
+obj-$(CONFIG_QCOM_SAHARA_PROTOCOL) += sahara/
diff --git a/drivers/soc/qcom/sahara/Kconfig b/drivers/soc/qcom/sahara/Kconfig
new file mode 100644
index 000000000000..4be90959736e
--- /dev/null
+++ b/drivers/soc/qcom/sahara/Kconfig
@@ -0,0 +1,17 @@
+config QCOM_SAHARA_PROTOCOL
+ tristate "Qualcomm Sahara protocol"
+ depends on MHI_BUS
+ select FW_LOADER_COMPRESS
+ select FW_LOADER_COMPRESS_XZ
+ select FW_LOADER_COMPRESS_ZSTD
+ help
+ The sahara protocol is primarily designed for transferring software
+ images from a host device to a target device using a simplified data
+ transfer mechanism over any physical link. However, the sahara
+ protocol does not support any authentication/validation of data sent
+ between devices. Such mechanism is beyond the scope of protocol.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the module will be
+ called qcom_sahara.
diff --git a/drivers/soc/qcom/sahara/Makefile b/drivers/soc/qcom/sahara/Makefile
new file mode 100644
index 000000000000..ad3922b30a31
--- /dev/null
+++ b/drivers/soc/qcom/sahara/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+obj-$(CONFIG_QCOM_SAHARA_PROTOCOL) := qcom_sahara.o
+
+qcom_sahara-y := sahara.o \
+ sahara_image_table.o
diff --git a/drivers/accel/qaic/sahara.c b/drivers/soc/qcom/sahara/sahara.c
similarity index 99%
rename from drivers/accel/qaic/sahara.c
rename to drivers/soc/qcom/sahara/sahara.c
index 7eae329396be..5e17d71a2d34 100644
--- a/drivers/accel/qaic/sahara.c
+++ b/drivers/soc/qcom/sahara/sahara.c
@@ -9,13 +9,12 @@
#include <linux/minmax.h>
#include <linux/mod_devicetable.h>
#include <linux/overflow.h>
+#include <linux/sahara.h>
+#include <linux/sahara_image_table_ops.h>
#include <linux/types.h>
#include <linux/vmalloc.h>
#include <linux/workqueue.h>
-#include "sahara.h"
-#include "sahara_image_table_ops.h"
-
#define SAHARA_HELLO_CMD 0x1 /* Min protocol version 1.0 */
#define SAHARA_HELLO_RESP_CMD 0x2 /* Min protocol version 1.0 */
#define SAHARA_READ_DATA_CMD 0x3 /* Min protocol version 1.0 */
@@ -814,8 +813,14 @@ int sahara_register(void)
{
return mhi_driver_register(&sahara_mhi_driver);
}
+module_init(sahara_register);
void sahara_unregister(void)
{
mhi_driver_unregister(&sahara_mhi_driver);
}
+module_exit(sahara_unregister);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Qualcomm Innovation Center, Inc");
+MODULE_DESCRIPTION("Sahara driver");
diff --git a/drivers/accel/qaic/sahara_image_table.c b/drivers/soc/qcom/sahara/sahara_image_table.c
similarity index 94%
rename from drivers/accel/qaic/sahara_image_table.c
rename to drivers/soc/qcom/sahara/sahara_image_table.c
index dd0793a33727..18f9b7a59f25 100644
--- a/drivers/accel/qaic/sahara_image_table.c
+++ b/drivers/soc/qcom/sahara/sahara_image_table.c
@@ -5,8 +5,8 @@
#include <linux/device.h>
#include <linux/list.h>
#include <linux/mutex.h>
+#include <linux/sahara_image_table_ops.h>
-#include "sahara_image_table_ops.h"
struct sahara_image_table_context {
struct list_head provider_list;
@@ -49,6 +49,7 @@ int sahara_register_image_table_provider(struct sahara_image_table_provider
return 0;
}
+EXPORT_SYMBOL_GPL(sahara_register_image_table_provider);
/**
* sahara_get_image_table - Get the image table for a given device name
@@ -78,6 +79,7 @@ const char * const *sahara_get_image_table(const char *dev_name)
return NULL;
}
+EXPORT_SYMBOL_GPL(sahara_get_image_table);
/**
* sahara_get_image_table_size - Get the size of the image table for a given
@@ -109,6 +111,7 @@ size_t sahara_get_image_table_size(const char *dev_name)
return 0;
}
+EXPORT_SYMBOL_GPL(sahara_get_image_table_size);
/**
* sahara_unregister_image_table_provider - Unregister an image table provider.
@@ -139,6 +142,7 @@ int sahara_unregister_image_table_provider(struct sahara_image_table_provider
return 0;
}
+EXPORT_SYMBOL_GPL(sahara_unregister_image_table_provider);
/**
* sahara_get_fw_folder_name - Retrieve the firmware folder name for a given
@@ -171,3 +175,4 @@ char *sahara_get_fw_folder_name(const char *dev_name)
return NULL;
}
+EXPORT_SYMBOL_GPL(sahara_get_fw_folder_name);
diff --git a/drivers/accel/qaic/sahara.h b/include/linux/sahara.h
similarity index 100%
rename from drivers/accel/qaic/sahara.h
rename to include/linux/sahara.h
diff --git a/drivers/accel/qaic/sahara_image_table_ops.h b/include/linux/sahara_image_table_ops.h
similarity index 100%
rename from drivers/accel/qaic/sahara_image_table_ops.h
rename to include/linux/sahara_image_table_ops.h
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v1 05/12] drivers: soc: qcom: Move Sahara driver to drivers/soc/qcom directory.
2025-08-25 10:19 ` [PATCH v1 05/12] drivers: soc: qcom: Move Sahara driver to drivers/soc/qcom directory Kishore Batta
@ 2025-08-25 22:12 ` Bjorn Andersson
0 siblings, 0 replies; 26+ messages in thread
From: Bjorn Andersson @ 2025-08-25 22:12 UTC (permalink / raw)
To: Kishore Batta
Cc: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio,
linux-arm-msm
On Mon, Aug 25, 2025 at 03:49:19PM +0530, Kishore Batta wrote:
> Move the Sahara protocol driver from the "drivers/accel" directory
> to the "drivers/soc/qcom" directory. This change makes the Sahara
> driver applicable to all Qualcomm devices, not just Qualcomm Accelerator
> devices. It also facilitates adding support for new devices. Client drivers
> can use the registration and deregistration functionalities of the Sahara
> driver as needed.
>
> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
> ---
> drivers/accel/qaic/Kconfig | 1 +
> drivers/accel/qaic/Makefile | 4 +---
> drivers/accel/qaic/mhi_controller.c | 2 +-
> drivers/accel/qaic/qaic_drv.c | 9 +--------
> drivers/soc/qcom/Kconfig | 6 ++++--
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/sahara/Kconfig | 17 +++++++++++++++++
No other drivers under drivers/soc/qcom/ has their own directory, I'm
not sure I see a reason for Sahara to be different.
> drivers/soc/qcom/sahara/Makefile | 6 ++++++
> .../{accel/qaic => soc/qcom/sahara}/sahara.c | 11 ++++++++---
> .../qcom/sahara}/sahara_image_table.c | 7 ++++++-
> {drivers/accel/qaic => include/linux}/sahara.h | 0
> .../linux}/sahara_image_table_ops.h | 0
> 12 files changed, 46 insertions(+), 18 deletions(-)
> create mode 100644 drivers/soc/qcom/sahara/Kconfig
> create mode 100644 drivers/soc/qcom/sahara/Makefile
> rename drivers/{accel/qaic => soc/qcom/sahara}/sahara.c (99%)
> rename drivers/{accel/qaic => soc/qcom/sahara}/sahara_image_table.c (94%)
> rename {drivers/accel/qaic => include/linux}/sahara.h (100%)
> rename {drivers/accel/qaic => include/linux}/sahara_image_table_ops.h (100%)
I was going to say something about other soc drivers living in
include/linux/soc/qcom/...
But that does touch upon another topic...drivers/soc/qcom is for
Qualcomm SoC drivers; and at least in the case of qaic, this driver
doesn't have anything to do with Qualcomm SoCs...
Given that this implementation only support, and is only ever going to
be used with, MHI. Perhaps drivers/bus/mhi would be a better home?
>
> diff --git a/drivers/accel/qaic/Kconfig b/drivers/accel/qaic/Kconfig
> index 5e405a19c157..5e2ac1ecede3 100644
> --- a/drivers/accel/qaic/Kconfig
> +++ b/drivers/accel/qaic/Kconfig
> @@ -8,6 +8,7 @@ config DRM_ACCEL_QAIC
> depends on DRM_ACCEL
> depends on PCI && HAS_IOMEM
> depends on MHI_BUS
> + select QCOM_SAHARA_PROTOCOL
> select CRC32
> help
> Enables driver for Qualcomm's Cloud AI accelerator PCIe cards that are
> diff --git a/drivers/accel/qaic/Makefile b/drivers/accel/qaic/Makefile
> index 586a6877e568..4ad84f7e2162 100644
> --- a/drivers/accel/qaic/Makefile
> +++ b/drivers/accel/qaic/Makefile
> @@ -11,8 +11,6 @@ qaic-y := \
> qaic_data.o \
> qaic_drv.o \
> qaic_ras.o \
> - qaic_timesync.o \
> - sahara.o \
> - sahara_image_table.o
> + qaic_timesync.o
>
> qaic-$(CONFIG_DEBUG_FS) += qaic_debugfs.o
> diff --git a/drivers/accel/qaic/mhi_controller.c b/drivers/accel/qaic/mhi_controller.c
> index 16c346e0e3b5..76beef6018a7 100644
> --- a/drivers/accel/qaic/mhi_controller.c
> +++ b/drivers/accel/qaic/mhi_controller.c
> @@ -9,11 +9,11 @@
> #include <linux/mhi.h>
> #include <linux/moduleparam.h>
> #include <linux/pci.h>
> +#include <linux/sahara_image_table_ops.h>
> #include <linux/sizes.h>
>
> #include "mhi_controller.h"
> #include "qaic.h"
> -#include "sahara_image_table_ops.h"
>
> #define MAX_RESET_TIME_SEC 25
>
> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index 5c4fab328003..a55e279411c3 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
> @@ -15,6 +15,7 @@
> #include <linux/msi.h>
> #include <linux/mutex.h>
> #include <linux/pci.h>
> +#include <linux/sahara.h>
> #include <linux/spinlock.h>
> #include <linux/workqueue.h>
> #include <linux/wait.h>
> @@ -31,7 +32,6 @@
> #include "qaic_debugfs.h"
> #include "qaic_ras.h"
> #include "qaic_timesync.h"
> -#include "sahara.h"
>
> MODULE_IMPORT_NS("DMA_BUF");
>
> @@ -682,12 +682,6 @@ static int __init qaic_init(void)
> goto free_pci;
> }
>
> - ret = sahara_register();
> - if (ret) {
> - pr_debug("qaic: sahara_register failed %d\n", ret);
> - goto free_mhi;
> - }
> -
> ret = qaic_sahara_register_image_tables();
> if (ret) {
> pr_debug("qaic: Image table registration failed %d\n", ret);
> @@ -737,7 +731,6 @@ static void __exit qaic_exit(void)
> qaic_ras_unregister();
> qaic_bootlog_unregister();
> qaic_timesync_deinit();
> - sahara_unregister();
> mhi_driver_unregister(&qaic_mhi_driver);
> pci_unregister_driver(&qaic_pci_driver);
> }
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 2caadbbcf830..7ea4cff9a679 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -295,8 +295,6 @@ config QCOM_PBS
> This module provides the APIs to the client drivers that wants to send the
> PBS trigger event to the PBS RAM.
>
> -endmenu
> -
> config QCOM_UBWC_CONFIG
> tristate
> help
> @@ -304,3 +302,7 @@ config QCOM_UBWC_CONFIG
> (UBWC) engines across various IP blocks, which need to be initialized
> with coherent configuration data. This module functions as a single
> source of truth for that information.
> +
> +source "drivers/soc/qcom/sahara/Kconfig"
> +
> +endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index b7f1d2a57367..99e490e3174e 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -40,3 +40,4 @@ qcom_ice-objs += ice.o
> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
> obj-$(CONFIG_QCOM_PBS) += qcom-pbs.o
> obj-$(CONFIG_QCOM_UBWC_CONFIG) += ubwc_config.o
> +obj-$(CONFIG_QCOM_SAHARA_PROTOCOL) += sahara/
> diff --git a/drivers/soc/qcom/sahara/Kconfig b/drivers/soc/qcom/sahara/Kconfig
> new file mode 100644
> index 000000000000..4be90959736e
> --- /dev/null
> +++ b/drivers/soc/qcom/sahara/Kconfig
> @@ -0,0 +1,17 @@
> +config QCOM_SAHARA_PROTOCOL
> + tristate "Qualcomm Sahara protocol"
It's bad practice to mix "select" and human selectable options. Drop the
"Qualcomm Sahara Protocol" and rely on the select to enable the driver.
> + depends on MHI_BUS
> + select FW_LOADER_COMPRESS
> + select FW_LOADER_COMPRESS_XZ
> + select FW_LOADER_COMPRESS_ZSTD
> + help
> + The sahara protocol is primarily designed for transferring software
> + images from a host device to a target device using a simplified data
> + transfer mechanism over any physical link. However, the sahara
> + protocol does not support any authentication/validation of data sent
> + between devices. Such mechanism is beyond the scope of protocol.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called qcom_sahara.
> diff --git a/drivers/soc/qcom/sahara/Makefile b/drivers/soc/qcom/sahara/Makefile
> new file mode 100644
> index 000000000000..ad3922b30a31
> --- /dev/null
> +++ b/drivers/soc/qcom/sahara/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_QCOM_SAHARA_PROTOCOL) := qcom_sahara.o
> +
> +qcom_sahara-y := sahara.o \
> + sahara_image_table.o
> diff --git a/drivers/accel/qaic/sahara.c b/drivers/soc/qcom/sahara/sahara.c
> similarity index 99%
> rename from drivers/accel/qaic/sahara.c
> rename to drivers/soc/qcom/sahara/sahara.c
> index 7eae329396be..5e17d71a2d34 100644
> --- a/drivers/accel/qaic/sahara.c
> +++ b/drivers/soc/qcom/sahara/sahara.c
> @@ -9,13 +9,12 @@
Make sure the style of the copyright comment matches the subsystem where
you move this driver to.
> #include <linux/minmax.h>
> #include <linux/mod_devicetable.h>
> #include <linux/overflow.h>
> +#include <linux/sahara.h>
> +#include <linux/sahara_image_table_ops.h>
> #include <linux/types.h>
> #include <linux/vmalloc.h>
> #include <linux/workqueue.h>
>
> -#include "sahara.h"
> -#include "sahara_image_table_ops.h"
> -
> #define SAHARA_HELLO_CMD 0x1 /* Min protocol version 1.0 */
> #define SAHARA_HELLO_RESP_CMD 0x2 /* Min protocol version 1.0 */
> #define SAHARA_READ_DATA_CMD 0x3 /* Min protocol version 1.0 */
> @@ -814,8 +813,14 @@ int sahara_register(void)
> {
> return mhi_driver_register(&sahara_mhi_driver);
> }
> +module_init(sahara_register);
>
> void sahara_unregister(void)
> {
> mhi_driver_unregister(&sahara_mhi_driver);
> }
> +module_exit(sahara_unregister);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Qualcomm Innovation Center, Inc");
Skip MODULE_AUTHOR, or correct it.
> +MODULE_DESCRIPTION("Sahara driver");
There's already another driver by the name "sahara", so be more
specific.
Regards,
Bjorn
> diff --git a/drivers/accel/qaic/sahara_image_table.c b/drivers/soc/qcom/sahara/sahara_image_table.c
> similarity index 94%
> rename from drivers/accel/qaic/sahara_image_table.c
> rename to drivers/soc/qcom/sahara/sahara_image_table.c
> index dd0793a33727..18f9b7a59f25 100644
> --- a/drivers/accel/qaic/sahara_image_table.c
> +++ b/drivers/soc/qcom/sahara/sahara_image_table.c
> @@ -5,8 +5,8 @@
> #include <linux/device.h>
> #include <linux/list.h>
> #include <linux/mutex.h>
> +#include <linux/sahara_image_table_ops.h>
>
> -#include "sahara_image_table_ops.h"
>
> struct sahara_image_table_context {
> struct list_head provider_list;
> @@ -49,6 +49,7 @@ int sahara_register_image_table_provider(struct sahara_image_table_provider
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(sahara_register_image_table_provider);
>
> /**
> * sahara_get_image_table - Get the image table for a given device name
> @@ -78,6 +79,7 @@ const char * const *sahara_get_image_table(const char *dev_name)
>
> return NULL;
> }
> +EXPORT_SYMBOL_GPL(sahara_get_image_table);
>
> /**
> * sahara_get_image_table_size - Get the size of the image table for a given
> @@ -109,6 +111,7 @@ size_t sahara_get_image_table_size(const char *dev_name)
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(sahara_get_image_table_size);
>
> /**
> * sahara_unregister_image_table_provider - Unregister an image table provider.
> @@ -139,6 +142,7 @@ int sahara_unregister_image_table_provider(struct sahara_image_table_provider
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(sahara_unregister_image_table_provider);
>
> /**
> * sahara_get_fw_folder_name - Retrieve the firmware folder name for a given
> @@ -171,3 +175,4 @@ char *sahara_get_fw_folder_name(const char *dev_name)
>
> return NULL;
> }
> +EXPORT_SYMBOL_GPL(sahara_get_fw_folder_name);
> diff --git a/drivers/accel/qaic/sahara.h b/include/linux/sahara.h
> similarity index 100%
> rename from drivers/accel/qaic/sahara.h
> rename to include/linux/sahara.h
> diff --git a/drivers/accel/qaic/sahara_image_table_ops.h b/include/linux/sahara_image_table_ops.h
> similarity index 100%
> rename from drivers/accel/qaic/sahara_image_table_ops.h
> rename to include/linux/sahara_image_table_ops.h
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 06/12] drivers: soc: qcom: Add support for Qualcomm QDU device.
2025-08-25 10:19 [PATCH v1 00/12] Sahara protocol enhancements Kishore Batta
` (4 preceding siblings ...)
2025-08-25 10:19 ` [PATCH v1 05/12] drivers: soc: qcom: Move Sahara driver to drivers/soc/qcom directory Kishore Batta
@ 2025-08-25 10:19 ` Kishore Batta
2025-08-25 23:24 ` Bjorn Andersson
2025-08-28 14:19 ` Krzysztof Kozlowski
2025-08-25 10:19 ` [PATCH v1 07/12] drivers: soc: qcom: Add sysfs support for DDR training data in Sahara Kishore Batta
` (5 subsequent siblings)
11 siblings, 2 replies; 26+ messages in thread
From: Kishore Batta @ 2025-08-25 10:19 UTC (permalink / raw)
To: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio
Cc: andersson, linux-arm-msm
Add support for the Qualcomm QDU device. The Qualcomm QDU driver acts
as a client driver to the Sahara protocol, including the QDU100-specific
image table and registering it during device initialization. The
registration of the image table is required to transfer the QDU100
specific firmware back to the device using Sahara protocol packets. The
MHI driver exposes a new channel name for the Qualcomm QDU100 device in
the pci_generic.c file, and the same channel support is added in the
Sahara driver.
Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
---
drivers/soc/qcom/Kconfig | 14 ++++++
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/qdu.c | 85 ++++++++++++++++++++++++++++++++
drivers/soc/qcom/sahara/sahara.c | 1 +
4 files changed, 101 insertions(+)
create mode 100644 drivers/soc/qcom/qdu.c
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 7ea4cff9a679..ffaaf6261c35 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -303,6 +303,20 @@ config QCOM_UBWC_CONFIG
with coherent configuration data. This module functions as a single
source of truth for that information.
+config QCOM_QDU
+ tristate "Qualcomm QDU driver"
+ select QCOM_SAHARA_PROTOCOL
+ help
+ This is a client driver which registers the device specific operations
+ with sahara protocol which is used to download firmware to Qualcomm
+ distributed unit device.
+ The Qualcomm QDU driver facilitates the registration of device
+ specific operations with the sahara protocol, enabling firmware
+ download to the QDU device.
+
+ To compile this driver as a module, choose M here: the module will be
+ called qdu.
+
source "drivers/soc/qcom/sahara/Kconfig"
endmenu
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 99e490e3174e..8d036edf304a 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -41,3 +41,4 @@ obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
obj-$(CONFIG_QCOM_PBS) += qcom-pbs.o
obj-$(CONFIG_QCOM_UBWC_CONFIG) += ubwc_config.o
obj-$(CONFIG_QCOM_SAHARA_PROTOCOL) += sahara/
+obj-$(CONFIG_QCOM_QDU) += qdu.o
diff --git a/drivers/soc/qcom/qdu.c b/drivers/soc/qcom/qdu.c
new file mode 100644
index 000000000000..afd8011793fa
--- /dev/null
+++ b/drivers/soc/qcom/qdu.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/sahara_image_table_ops.h>
+
+static const char * const qdu100_image_table[] = {
+ [5] = "qcom/qdu100/uefi.elf",
+ [8] = "qcom/qdu100/qdsp6sw.mbn",
+ [16] = "qcom/qdu100/efs1.bin",
+ [17] = "qcom/qdu100/efs2.bin",
+ [20] = "qcom/qdu100/efs3.bin",
+ [23] = "qcom/qdu100/aop.mbn",
+ [25] = "qcom/qdu100/tz.mbn",
+ [29] = "qcom/qdu100/zeros_1sector.bin",
+ [33] = "qcom/qdu100/hypvm.mbn",
+ [34] = "qcom/qdu100/mdmddr.mbn",
+ [36] = "qcom/qdu100/multi_image_qti.mbn",
+ [37] = "qcom/qdu100/multi_image.mbn",
+ [38] = "qcom/qdu100/xbl_config.elf",
+ [39] = "qcom/qdu100/abl_userdebug.elf",
+ [40] = "qcom/qdu100/zeros_1sector.bin",
+ [41] = "qcom/qdu100/devcfg.mbn",
+ [42] = "qcom/qdu100/zeros_1sector.bin",
+ [43] = "qcom/qdu100/kernel_boot.elf",
+ [45] = "qcom/qdu100/tools_l.elf",
+ [46] = "qcom/qdu100/Quantum.elf",
+ [47] = "qcom/qdu100/quest.elf",
+ [48] = "qcom/qdu100/xbl_ramdump.elf",
+ [49] = "qcom/qdu100/shrm.elf",
+ [50] = "qcom/qdu100/cpucp.elf",
+ [51] = "qcom/qdu100/aop_devcfg.mbn",
+ [52] = "qcom/qdu100/fw_csm_gsi_3.0.elf",
+ [53] = "qcom/qdu100/qdsp6sw_dtbs.elf",
+ [54] = "qcom/qdu100/qupv3fw.elf",
+};
+
+static struct sahara_image_table_provider qdu100_provider = {
+ .image_table = qdu100_image_table,
+ .image_table_size = ARRAY_SIZE(qdu100_image_table),
+ .dev_name = "qcom-qdu100",
+ .fw_folder_name = "qdu100",
+ .list = LIST_HEAD_INIT(qdu100_provider.list)
+};
+
+static struct sahara_image_table_provider *qdu_providers[] = {
+ &qdu100_provider,
+};
+
+static int __init qdu_init(void)
+{
+ int ret;
+
+ for (int i = 0; i < ARRAY_SIZE(qdu_providers); i++) {
+ ret = sahara_register_image_table_provider(qdu_providers[i]);
+ if (ret) {
+ pr_err("qdu: Failed to register image table %d\n", ret);
+
+ /* Rollback previously registered providers */
+ while (--i >= 0)
+ sahara_unregister_image_table_provider(qdu_providers[i]);
+ return ret;
+ }
+ }
+ return 0;
+}
+module_init(qdu_init);
+
+static void __exit qdu_exit(void)
+{
+ int ret;
+
+ for (int i = 0; i < ARRAY_SIZE(qdu_providers); i++) {
+ ret = sahara_unregister_image_table_provider(qdu_providers[i]);
+ if (ret)
+ pr_err("qdu: Failed to unregister image table %d\n", ret);
+ }
+}
+module_exit(qdu_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Qualcomm distributed unit driver");
diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
index 5e17d71a2d34..b07f6bd0e19f 100644
--- a/drivers/soc/qcom/sahara/sahara.c
+++ b/drivers/soc/qcom/sahara/sahara.c
@@ -795,6 +795,7 @@ static void sahara_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result
static const struct mhi_device_id sahara_mhi_match_table[] = {
{ .chan = "QAIC_SAHARA", },
+ { .chan = "SAHARA", },
{},
};
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v1 06/12] drivers: soc: qcom: Add support for Qualcomm QDU device.
2025-08-25 10:19 ` [PATCH v1 06/12] drivers: soc: qcom: Add support for Qualcomm QDU device Kishore Batta
@ 2025-08-25 23:24 ` Bjorn Andersson
2025-08-28 12:48 ` Kishore Batta
2025-08-28 14:19 ` Krzysztof Kozlowski
1 sibling, 1 reply; 26+ messages in thread
From: Bjorn Andersson @ 2025-08-25 23:24 UTC (permalink / raw)
To: Kishore Batta
Cc: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio,
linux-arm-msm
On Mon, Aug 25, 2025 at 03:49:20PM +0530, Kishore Batta wrote:
> Add support for the Qualcomm QDU device. The Qualcomm QDU driver acts
> as a client driver to the Sahara protocol, including the QDU100-specific
> image table and registering it during device initialization.
"including the QDU100-specific image table"? That's not "including" it's
the only thing it does.
> The
> registration of the image table is required to transfer the QDU100
> specific firmware back to the device using Sahara protocol packets.
Compare above 5-6 lines with:
"The QDU device is flash-less and uses Sahara to load its firmware,
provide the table of firmware files."
> The
> MHI driver exposes a new channel name for the Qualcomm QDU100 device in
> the pci_generic.c file, and the same channel support is added in the
> Sahara driver.
Which MHI driver exposes a new channel name? Please rephrase this and
perhaps split it into a separate patch?
>
> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
> ---
> drivers/soc/qcom/Kconfig | 14 ++++++
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qdu.c | 85 ++++++++++++++++++++++++++++++++
> drivers/soc/qcom/sahara/sahara.c | 1 +
> 4 files changed, 101 insertions(+)
> create mode 100644 drivers/soc/qcom/qdu.c
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 7ea4cff9a679..ffaaf6261c35 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -303,6 +303,20 @@ config QCOM_UBWC_CONFIG
> with coherent configuration data. This module functions as a single
> source of truth for that information.
>
> +config QCOM_QDU
> + tristate "Qualcomm QDU driver"
> + select QCOM_SAHARA_PROTOCOL
> + help
> + This is a client driver which registers the device specific operations
> + with sahara protocol which is used to download firmware to Qualcomm
> + distributed unit device.
> + The Qualcomm QDU driver facilitates the registration of device
> + specific operations with the sahara protocol, enabling firmware
> + download to the QDU device.
This makes it sound like there's a bunch going on in this driver, is
more of this coming?
> +
> + To compile this driver as a module, choose M here: the module will be
> + called qdu.
> +
> source "drivers/soc/qcom/sahara/Kconfig"
>
> endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 99e490e3174e..8d036edf304a 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -41,3 +41,4 @@ obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
> obj-$(CONFIG_QCOM_PBS) += qcom-pbs.o
> obj-$(CONFIG_QCOM_UBWC_CONFIG) += ubwc_config.o
> obj-$(CONFIG_QCOM_SAHARA_PROTOCOL) += sahara/
> +obj-$(CONFIG_QCOM_QDU) += qdu.o
This list was somewhat an approximation of being in alphabetical
order...
> diff --git a/drivers/soc/qcom/qdu.c b/drivers/soc/qcom/qdu.c
> new file mode 100644
> index 000000000000..afd8011793fa
> --- /dev/null
> +++ b/drivers/soc/qcom/qdu.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. */
Look at other drivers in drivers/soc/qcom...they don't format this
comment like this.
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/sahara_image_table_ops.h>
> +
> +static const char * const qdu100_image_table[] = {
> + [5] = "qcom/qdu100/uefi.elf",
> + [8] = "qcom/qdu100/qdsp6sw.mbn",
> + [16] = "qcom/qdu100/efs1.bin",
> + [17] = "qcom/qdu100/efs2.bin",
> + [20] = "qcom/qdu100/efs3.bin",
> + [23] = "qcom/qdu100/aop.mbn",
> + [25] = "qcom/qdu100/tz.mbn",
> + [29] = "qcom/qdu100/zeros_1sector.bin",
> + [33] = "qcom/qdu100/hypvm.mbn",
> + [34] = "qcom/qdu100/mdmddr.mbn",
> + [36] = "qcom/qdu100/multi_image_qti.mbn",
> + [37] = "qcom/qdu100/multi_image.mbn",
> + [38] = "qcom/qdu100/xbl_config.elf",
> + [39] = "qcom/qdu100/abl_userdebug.elf",
> + [40] = "qcom/qdu100/zeros_1sector.bin",
> + [41] = "qcom/qdu100/devcfg.mbn",
> + [42] = "qcom/qdu100/zeros_1sector.bin",
> + [43] = "qcom/qdu100/kernel_boot.elf",
> + [45] = "qcom/qdu100/tools_l.elf",
> + [46] = "qcom/qdu100/Quantum.elf",
> + [47] = "qcom/qdu100/quest.elf",
> + [48] = "qcom/qdu100/xbl_ramdump.elf",
> + [49] = "qcom/qdu100/shrm.elf",
> + [50] = "qcom/qdu100/cpucp.elf",
> + [51] = "qcom/qdu100/aop_devcfg.mbn",
> + [52] = "qcom/qdu100/fw_csm_gsi_3.0.elf",
> + [53] = "qcom/qdu100/qdsp6sw_dtbs.elf",
> + [54] = "qcom/qdu100/qupv3fw.elf",
> +};
> +
> +static struct sahara_image_table_provider qdu100_provider = {
> + .image_table = qdu100_image_table,
> + .image_table_size = ARRAY_SIZE(qdu100_image_table),
> + .dev_name = "qcom-qdu100",
> + .fw_folder_name = "qdu100",
> + .list = LIST_HEAD_INIT(qdu100_provider.list)
> +};
> +
> +static struct sahara_image_table_provider *qdu_providers[] = {
> + &qdu100_provider,
> +};
> +
> +static int __init qdu_init(void)
So patch 2, 3, and 4 just laid the groundwork, so that you could add a
new "driver" with the only purpose of carrying an array of firmware
names and the extra code to register it with the Sahara driver.
Why don't you just put the qdu100_image_table[] array in sahara.c and
save us a whole bunch of changes?
> +{
> + int ret;
> +
> + for (int i = 0; i < ARRAY_SIZE(qdu_providers); i++) {
Future proof! Please don't.
Regards,
Bjorn
> + ret = sahara_register_image_table_provider(qdu_providers[i]);
> + if (ret) {
> + pr_err("qdu: Failed to register image table %d\n", ret);
> +
> + /* Rollback previously registered providers */
> + while (--i >= 0)
> + sahara_unregister_image_table_provider(qdu_providers[i]);
> + return ret;
> + }
> + }
> + return 0;
> +}
> +module_init(qdu_init);
> +
> +static void __exit qdu_exit(void)
> +{
> + int ret;
> +
> + for (int i = 0; i < ARRAY_SIZE(qdu_providers); i++) {
> + ret = sahara_unregister_image_table_provider(qdu_providers[i]);
> + if (ret)
> + pr_err("qdu: Failed to unregister image table %d\n", ret);
> + }
> +}
> +module_exit(qdu_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Qualcomm distributed unit driver");
> diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
> index 5e17d71a2d34..b07f6bd0e19f 100644
> --- a/drivers/soc/qcom/sahara/sahara.c
> +++ b/drivers/soc/qcom/sahara/sahara.c
> @@ -795,6 +795,7 @@ static void sahara_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result
>
> static const struct mhi_device_id sahara_mhi_match_table[] = {
> { .chan = "QAIC_SAHARA", },
> + { .chan = "SAHARA", },
> {},
> };
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 06/12] drivers: soc: qcom: Add support for Qualcomm QDU device.
2025-08-25 23:24 ` Bjorn Andersson
@ 2025-08-28 12:48 ` Kishore Batta
0 siblings, 0 replies; 26+ messages in thread
From: Kishore Batta @ 2025-08-28 12:48 UTC (permalink / raw)
To: Bjorn Andersson
Cc: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio,
linux-arm-msm
On 8/26/2025 4:54 AM, Bjorn Andersson wrote:
> On Mon, Aug 25, 2025 at 03:49:20PM +0530, Kishore Batta wrote:
>> Add support for the Qualcomm QDU device. The Qualcomm QDU driver acts
>> as a client driver to the Sahara protocol, including the QDU100-specific
>> image table and registering it during device initialization.
> "including the QDU100-specific image table"? That's not "including" it's
> the only thing it does.
>
>> The
>> registration of the image table is required to transfer the QDU100
>> specific firmware back to the device using Sahara protocol packets.
> Compare above 5-6 lines with:
>
> "The QDU device is flash-less and uses Sahara to load its firmware,
> provide the table of firmware files."
>
>> The
>> MHI driver exposes a new channel name for the Qualcomm QDU100 device in
>> the pci_generic.c file, and the same channel support is added in the
>> Sahara driver.
> Which MHI driver exposes a new channel name? Please rephrase this and
> perhaps split it into a separate patch?
>
>> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
>> ---
>> drivers/soc/qcom/Kconfig | 14 ++++++
>> drivers/soc/qcom/Makefile | 1 +
>> drivers/soc/qcom/qdu.c | 85 ++++++++++++++++++++++++++++++++
>> drivers/soc/qcom/sahara/sahara.c | 1 +
>> 4 files changed, 101 insertions(+)
>> create mode 100644 drivers/soc/qcom/qdu.c
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 7ea4cff9a679..ffaaf6261c35 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -303,6 +303,20 @@ config QCOM_UBWC_CONFIG
>> with coherent configuration data. This module functions as a single
>> source of truth for that information.
>>
>> +config QCOM_QDU
>> + tristate "Qualcomm QDU driver"
>> + select QCOM_SAHARA_PROTOCOL
>> + help
>> + This is a client driver which registers the device specific operations
>> + with sahara protocol which is used to download firmware to Qualcomm
>> + distributed unit device.
>> + The Qualcomm QDU driver facilitates the registration of device
>> + specific operations with the sahara protocol, enabling firmware
>> + download to the QDU device.
> This makes it sound like there's a bunch going on in this driver, is
> more of this coming?
>
>> +
>> + To compile this driver as a module, choose M here: the module will be
>> + called qdu.
>> +
>> source "drivers/soc/qcom/sahara/Kconfig"
>>
>> endmenu
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 99e490e3174e..8d036edf304a 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -41,3 +41,4 @@ obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
>> obj-$(CONFIG_QCOM_PBS) += qcom-pbs.o
>> obj-$(CONFIG_QCOM_UBWC_CONFIG) += ubwc_config.o
>> obj-$(CONFIG_QCOM_SAHARA_PROTOCOL) += sahara/
>> +obj-$(CONFIG_QCOM_QDU) += qdu.o
> This list was somewhat an approximation of being in alphabetical
> order...
>
>> diff --git a/drivers/soc/qcom/qdu.c b/drivers/soc/qcom/qdu.c
>> new file mode 100644
>> index 000000000000..afd8011793fa
>> --- /dev/null
>> +++ b/drivers/soc/qcom/qdu.c
>> @@ -0,0 +1,85 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +/* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. */
> Look at other drivers in drivers/soc/qcom...they don't format this
> comment like this.
>
>> +
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/sahara_image_table_ops.h>
>> +
>> +static const char * const qdu100_image_table[] = {
>> + [5] = "qcom/qdu100/uefi.elf",
>> + [8] = "qcom/qdu100/qdsp6sw.mbn",
>> + [16] = "qcom/qdu100/efs1.bin",
>> + [17] = "qcom/qdu100/efs2.bin",
>> + [20] = "qcom/qdu100/efs3.bin",
>> + [23] = "qcom/qdu100/aop.mbn",
>> + [25] = "qcom/qdu100/tz.mbn",
>> + [29] = "qcom/qdu100/zeros_1sector.bin",
>> + [33] = "qcom/qdu100/hypvm.mbn",
>> + [34] = "qcom/qdu100/mdmddr.mbn",
>> + [36] = "qcom/qdu100/multi_image_qti.mbn",
>> + [37] = "qcom/qdu100/multi_image.mbn",
>> + [38] = "qcom/qdu100/xbl_config.elf",
>> + [39] = "qcom/qdu100/abl_userdebug.elf",
>> + [40] = "qcom/qdu100/zeros_1sector.bin",
>> + [41] = "qcom/qdu100/devcfg.mbn",
>> + [42] = "qcom/qdu100/zeros_1sector.bin",
>> + [43] = "qcom/qdu100/kernel_boot.elf",
>> + [45] = "qcom/qdu100/tools_l.elf",
>> + [46] = "qcom/qdu100/Quantum.elf",
>> + [47] = "qcom/qdu100/quest.elf",
>> + [48] = "qcom/qdu100/xbl_ramdump.elf",
>> + [49] = "qcom/qdu100/shrm.elf",
>> + [50] = "qcom/qdu100/cpucp.elf",
>> + [51] = "qcom/qdu100/aop_devcfg.mbn",
>> + [52] = "qcom/qdu100/fw_csm_gsi_3.0.elf",
>> + [53] = "qcom/qdu100/qdsp6sw_dtbs.elf",
>> + [54] = "qcom/qdu100/qupv3fw.elf",
>> +};
>> +
>> +static struct sahara_image_table_provider qdu100_provider = {
>> + .image_table = qdu100_image_table,
>> + .image_table_size = ARRAY_SIZE(qdu100_image_table),
>> + .dev_name = "qcom-qdu100",
>> + .fw_folder_name = "qdu100",
>> + .list = LIST_HEAD_INIT(qdu100_provider.list)
>> +};
>> +
>> +static struct sahara_image_table_provider *qdu_providers[] = {
>> + &qdu100_provider,
>> +};
>> +
>> +static int __init qdu_init(void)
> So patch 2, 3, and 4 just laid the groundwork, so that you could add a
> new "driver" with the only purpose of carrying an array of firmware
> names and the extra code to register it with the Sahara driver.
>
> Why don't you just put the qdu100_image_table[] array in sahara.c and
> save us a whole bunch of changes?
Hi Bjorn,
I thought that embedding the QDU100 image table directly into sahara driver
would risk coupling it too tightly to a specific device, which goes against
the goal of keeping Sahara generic and protocol focused.
To address this, I have restructured the patch to introduce minimal MHI
client driver(drivers/bus/mhi/qdu.c)
that solely registers the QDU100 image table using sahara registration
APIs introduced earlier
in the series. This keeps sahara clean and device agnostic while still
enabling firmware loading
for QDU and AIC devices.
The new driver is conditionally compiled using CONFIG_MHI_QDU and it
follows the same pattern as AIC module
firmware registration mechanism.
Please let me know if this direction looks good to you, and I'll include
the updated patch in the next version.
Thanks,
- Kishore.
>
>> +{
>> + int ret;
>> +
>> + for (int i = 0; i < ARRAY_SIZE(qdu_providers); i++) {
> Future proof! Please don't.
>
> Regards,
> Bjorn
>
>> + ret = sahara_register_image_table_provider(qdu_providers[i]);
>> + if (ret) {
>> + pr_err("qdu: Failed to register image table %d\n", ret);
>> +
>> + /* Rollback previously registered providers */
>> + while (--i >= 0)
>> + sahara_unregister_image_table_provider(qdu_providers[i]);
>> + return ret;
>> + }
>> + }
>> + return 0;
>> +}
>> +module_init(qdu_init);
>> +
>> +static void __exit qdu_exit(void)
>> +{
>> + int ret;
>> +
>> + for (int i = 0; i < ARRAY_SIZE(qdu_providers); i++) {
>> + ret = sahara_unregister_image_table_provider(qdu_providers[i]);
>> + if (ret)
>> + pr_err("qdu: Failed to unregister image table %d\n", ret);
>> + }
>> +}
>> +module_exit(qdu_exit);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Qualcomm distributed unit driver");
>> diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
>> index 5e17d71a2d34..b07f6bd0e19f 100644
>> --- a/drivers/soc/qcom/sahara/sahara.c
>> +++ b/drivers/soc/qcom/sahara/sahara.c
>> @@ -795,6 +795,7 @@ static void sahara_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result
>>
>> static const struct mhi_device_id sahara_mhi_match_table[] = {
>> { .chan = "QAIC_SAHARA", },
>> + { .chan = "SAHARA", },
>> {},
>> };
>>
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 06/12] drivers: soc: qcom: Add support for Qualcomm QDU device.
2025-08-25 10:19 ` [PATCH v1 06/12] drivers: soc: qcom: Add support for Qualcomm QDU device Kishore Batta
2025-08-25 23:24 ` Bjorn Andersson
@ 2025-08-28 14:19 ` Krzysztof Kozlowski
1 sibling, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2025-08-28 14:19 UTC (permalink / raw)
To: Kishore Batta, jeff.hugo, bjorn.andersson, konradybcio,
konrad.dybcio
Cc: andersson, linux-arm-msm
On 25/08/2025 12:19, Kishore Batta wrote:
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 99e490e3174e..8d036edf304a 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -41,3 +41,4 @@ obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
> obj-$(CONFIG_QCOM_PBS) += qcom-pbs.o
> obj-$(CONFIG_QCOM_UBWC_CONFIG) += ubwc_config.o
> obj-$(CONFIG_QCOM_SAHARA_PROTOCOL) += sahara/
> +obj-$(CONFIG_QCOM_QDU) += qdu.o
> diff --git a/drivers/soc/qcom/qdu.c b/drivers/soc/qcom/qdu.c
> new file mode 100644
> index 000000000000..afd8011793fa
> --- /dev/null
> +++ b/drivers/soc/qcom/qdu.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/sahara_image_table_ops.h>
> +
> +static const char * const qdu100_image_table[] = {
> + [5] = "qcom/qdu100/uefi.elf",
> + [8] = "qcom/qdu100/qdsp6sw.mbn",
> + [16] = "qcom/qdu100/efs1.bin",
> + [17] = "qcom/qdu100/efs2.bin",
> + [20] = "qcom/qdu100/efs3.bin",
> + [23] = "qcom/qdu100/aop.mbn",
> + [25] = "qcom/qdu100/tz.mbn",
> + [29] = "qcom/qdu100/zeros_1sector.bin",
> + [33] = "qcom/qdu100/hypvm.mbn",
> + [34] = "qcom/qdu100/mdmddr.mbn",
> + [36] = "qcom/qdu100/multi_image_qti.mbn",
> + [37] = "qcom/qdu100/multi_image.mbn",
> + [38] = "qcom/qdu100/xbl_config.elf",
> + [39] = "qcom/qdu100/abl_userdebug.elf",
> + [40] = "qcom/qdu100/zeros_1sector.bin",
> + [41] = "qcom/qdu100/devcfg.mbn",
> + [42] = "qcom/qdu100/zeros_1sector.bin",
> + [43] = "qcom/qdu100/kernel_boot.elf",
We should not accept this code because corresponding linux-firmware
release was not properly authorized in Qualcomm. All of above might change.
I can give more details in private.
BTW, please drop full stop from your subjects. Subject is not a sentence.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 07/12] drivers: soc: qcom: Add sysfs support for DDR training data in Sahara.
2025-08-25 10:19 [PATCH v1 00/12] Sahara protocol enhancements Kishore Batta
` (5 preceding siblings ...)
2025-08-25 10:19 ` [PATCH v1 06/12] drivers: soc: qcom: Add support for Qualcomm QDU device Kishore Batta
@ 2025-08-25 10:19 ` Kishore Batta
2025-08-25 22:37 ` Bjorn Andersson
2025-08-25 10:19 ` [PATCH v1 08/12] drivers: soc: qcom: Support Sahara command mode packets(READY and EXECUTE) Kishore Batta
` (4 subsequent siblings)
11 siblings, 1 reply; 26+ messages in thread
From: Kishore Batta @ 2025-08-25 10:19 UTC (permalink / raw)
To: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio
Cc: andersson, linux-arm-msm
Add the Sahara training data structure and populate the DDR training data
sysfs node. During device boot, the device performs DDR training and sends
the training data back to the host once complete. The host exposes this
data to userspace via the sysfs interface. The "ddr_training_data" sysfs
node will be present in the MHI controller node (e.g., mhi0, mhi1) along
with other existing sysfs nodes at /sys/bus/mhi/devices/mhi*/.
When the host reboots, a userspace service is triggered via an udev rule to
read the training data from the sysfs entry. The service then copies the
valid training data to the image firmware filesystem. This change includes
the structures added for DDR training data and embeds them in the
sahara_dev_driver_data structure. It also populates the sysfs attributes
for DDR training data.
Userspace service - https://github.com/qualcomm/csm-utils
Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
---
drivers/soc/qcom/sahara/sahara.c | 109 ++++++++++++++++++++++++++++++-
1 file changed, 108 insertions(+), 1 deletion(-)
diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
index b07f6bd0e19f..df103473af3a 100644
--- a/drivers/soc/qcom/sahara/sahara.c
+++ b/drivers/soc/qcom/sahara/sahara.c
@@ -60,6 +60,12 @@
#define SAHARA_MEM_DEBUG64_LENGTH 0x18
#define SAHARA_MEM_READ64_LENGTH 0x18
+struct sahara_dev_trng_data {
+ void *trng_data;
+ u64 trng_data_sz;
+ bool receiving_trng_data;
+};
+
struct sahara_packet {
__le32 cmd;
__le32 length;
@@ -177,6 +183,58 @@ struct sahara_context {
bool is_mem_dump_mode;
};
+struct sahara_dev_driver_data {
+ struct bin_attribute ddr_attr;
+ struct sahara_dev_trng_data *sdev;
+ struct sahara_context *context;
+};
+
+/* Exposes DDR training data via sysfs binary attribute for user-space access */
+static ssize_t ddr_training_read(struct file *filp, struct kobject *kobj,
+ const struct bin_attribute *attr, char *buf,
+ loff_t offset, size_t count)
+{
+ struct sahara_dev_driver_data *sahara_data;
+ struct sahara_dev_trng_data *sdev;
+ size_t data_size;
+
+ sahara_data = container_of(attr, struct sahara_dev_driver_data, ddr_attr);
+
+ if (!sahara_data)
+ return -EIO;
+
+ sdev = sahara_data->sdev;
+
+ if (!sdev || !sdev->trng_data)
+ return -EIO;
+
+ data_size = attr->size;
+
+ if (data_size == 0)
+ return 0;
+
+ if (offset >= data_size)
+ return -EINVAL;
+
+ if (count > data_size - offset)
+ count = data_size - offset;
+
+ /* Copy the training data into the buffer */
+ memcpy(buf, (sdev->trng_data + offset), count);
+
+ /* Free memory after last read */
+ if (offset + count >= data_size) {
+ kfree(sdev->trng_data);
+ sdev->trng_data = NULL;
+ kfree(sdev);
+ sdev = NULL;
+ kfree(sahara_data);
+ sahara_data = NULL;
+ }
+
+ return count;
+}
+
static int sahara_find_image(struct sahara_context *context, u32 image_id)
{
int ret;
@@ -703,11 +761,42 @@ static void sahara_dump_processing(struct work_struct *work)
sahara_send_reset(context);
}
+static int sahara_dev_populate_attribute(struct sahara_dev_driver_data *sahara_data)
+{
+ int ret;
+ struct sahara_context *context = sahara_data->context;
+
+ /* DDR training data attribute */
+ sahara_data->ddr_attr.attr.name = "ddr_training_data";
+ sahara_data->ddr_attr.attr.mode = 0444;
+ sahara_data->ddr_attr.read = ddr_training_read;
+
+ /* Size is populated during device bootup */
+ sahara_data->ddr_attr.size = 0;
+ sahara_data->ddr_attr.write = NULL;
+
+ /*
+ * Remove any existing sysfs binary attribute to avoid stale entries
+ * during warm boot or reinitialization. This ensures clean state before
+ * re-creating the attribute.
+ */
+ device_remove_bin_file(&context->mhi_dev->mhi_cntrl->mhi_dev->dev,
+ &sahara_data->ddr_attr);
+
+ /* Create the binary attribute */
+ ret = device_create_bin_file(&context->mhi_dev->mhi_cntrl->mhi_dev->dev,
+ &sahara_data->ddr_attr);
+
+ return ret;
+}
+
static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_id *id)
{
struct sahara_context *context;
int ret;
int i;
+ struct sahara_dev_driver_data *sahara_data;
+ struct sahara_dev_trng_data *sdev;
context = devm_kzalloc(&mhi_dev->dev, sizeof(*context), GFP_KERNEL);
if (!context)
@@ -717,6 +806,17 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
if (!context->rx)
return -ENOMEM;
+ sahara_data = kzalloc(sizeof(*sahara_data), GFP_KERNEL);
+ if (!sahara_data)
+ return -ENOMEM;
+
+ sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
+ if (!sdev)
+ return -ENOMEM;
+
+ sahara_data->context = context;
+ sahara_data->sdev = sdev;
+
/*
* AIC100 defines SAHARA_TRANSFER_MAX_SIZE as the largest value it
* will request for READ_DATA. This is larger than
@@ -749,7 +849,14 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
return -EINVAL;
context->active_image_id = SAHARA_IMAGE_ID_NONE;
- dev_set_drvdata(&mhi_dev->dev, context);
+ dev_set_drvdata(&mhi_dev->dev, sahara_data);
+
+ ret = sahara_dev_populate_attribute(sahara_data);
+ if (ret) {
+ dev_err(&context->mhi_dev->dev,
+ "Failed to populate bin attribute: %d\n", ret);
+ return ret;
+ }
ret = mhi_prepare_for_transfer(mhi_dev);
if (ret)
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v1 07/12] drivers: soc: qcom: Add sysfs support for DDR training data in Sahara.
2025-08-25 10:19 ` [PATCH v1 07/12] drivers: soc: qcom: Add sysfs support for DDR training data in Sahara Kishore Batta
@ 2025-08-25 22:37 ` Bjorn Andersson
0 siblings, 0 replies; 26+ messages in thread
From: Bjorn Andersson @ 2025-08-25 22:37 UTC (permalink / raw)
To: Kishore Batta
Cc: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio,
linux-arm-msm
On Mon, Aug 25, 2025 at 03:49:21PM +0530, Kishore Batta wrote:
No "drivers: " prefix in subject please.
> Add the Sahara training data structure and populate the DDR training data
> sysfs node.
Start with a problem description.
> During device boot, the device performs DDR training and sends
> the training data back to the host once complete.
"The device"? All devices, some devices?
> The host exposes this
> data to userspace via the sysfs interface.
I've read three sentences, you've given me breadcrumbs of what's going
on...but you're forcing me to guess how these three things fit together
and what you're trying to achieve...
> The "ddr_training_data" sysfs
> node will be present in the MHI controller node (e.g., mhi0, mhi1) along
> with other existing sysfs nodes at /sys/bus/mhi/devices/mhi*/.
>
> When the host reboots, a userspace service is triggered via an udev rule to
> read the training data from the sysfs entry.
This describes one possible way to do that, but it's not the job of the
kernel to dictate how this is implemented. You should describe the
expected work to be performed by userspace, and you may suggest how
that could be implemented.
> The service then copies the
> valid training data to the image firmware filesystem.
This sentence doesn't make sense to a general Linux user, because
there's no separate "image firmware filesystem".
> This change includes
> the structures added for DDR training data and embeds them in the
> sahara_dev_driver_data structure. It also populates the sysfs attributes
> for DDR training data.
This half of the paragraph isn't directly related to the implementation
of the user space service, so better split it out in a paragraph of its
own.
>
> Userspace service - https://github.com/qualcomm/csm-utils
>
> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
> ---
> drivers/soc/qcom/sahara/sahara.c | 109 ++++++++++++++++++++++++++++++-
> 1 file changed, 108 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
> index b07f6bd0e19f..df103473af3a 100644
> --- a/drivers/soc/qcom/sahara/sahara.c
> +++ b/drivers/soc/qcom/sahara/sahara.c
> @@ -60,6 +60,12 @@
> #define SAHARA_MEM_DEBUG64_LENGTH 0x18
> #define SAHARA_MEM_READ64_LENGTH 0x18
>
> +struct sahara_dev_trng_data {
trng - "True Random Number Generator"?
> + void *trng_data;
> + u64 trng_data_sz;
> + bool receiving_trng_data;
> +};
> +
> struct sahara_packet {
> __le32 cmd;
> __le32 length;
> @@ -177,6 +183,58 @@ struct sahara_context {
> bool is_mem_dump_mode;
> };
>
> +struct sahara_dev_driver_data {
> + struct bin_attribute ddr_attr;
> + struct sahara_dev_trng_data *sdev;
"sdev" as an abbreviation for "sahara device training data"? I would
have guessed it related tot he "sahara device driver data"...
Why do you have two separate structs for these?
> + struct sahara_context *context;
> +};
> +
> +/* Exposes DDR training data via sysfs binary attribute for user-space access */
> +static ssize_t ddr_training_read(struct file *filp, struct kobject *kobj,
> + const struct bin_attribute *attr, char *buf,
> + loff_t offset, size_t count)
> +{
> + struct sahara_dev_driver_data *sahara_data;
> + struct sahara_dev_trng_data *sdev;
> + size_t data_size;
> +
> + sahara_data = container_of(attr, struct sahara_dev_driver_data, ddr_attr);
> +
> + if (!sahara_data)
> + return -EIO;
> +
> + sdev = sahara_data->sdev;
> +
> + if (!sdev || !sdev->trng_data)
This isn't assigned anywhere...
> + return -EIO;
> +
> + data_size = attr->size;
> +
> + if (data_size == 0)
> + return 0;
> +
> + if (offset >= data_size)
> + return -EINVAL;
> +
> + if (count > data_size - offset)
> + count = data_size - offset;
> +
> + /* Copy the training data into the buffer */
> + memcpy(buf, (sdev->trng_data + offset), count);
> +
> + /* Free memory after last read */
> + if (offset + count >= data_size) {
> + kfree(sdev->trng_data);
> + sdev->trng_data = NULL;
> + kfree(sdev);
Allowing the data to be read only one time and then failing subsequent
reads is going to be confusing to people. Imagine debugging this and
depending on how fast you can hexdump the attribute you either break the
userspace thing or you are unable to catch the data.
> + sdev = NULL;
> + kfree(sahara_data);
But you did device_create_bin_file() on &sahara_data->ddr_attr...what
happens now?
> + sahara_data = NULL;
> + }
> +
> + return count;
> +}
> +
> static int sahara_find_image(struct sahara_context *context, u32 image_id)
> {
> int ret;
> @@ -703,11 +761,42 @@ static void sahara_dump_processing(struct work_struct *work)
> sahara_send_reset(context);
> }
>
> +static int sahara_dev_populate_attribute(struct sahara_dev_driver_data *sahara_data)
> +{
> + int ret;
> + struct sahara_context *context = sahara_data->context;
> +
> + /* DDR training data attribute */
> + sahara_data->ddr_attr.attr.name = "ddr_training_data";
> + sahara_data->ddr_attr.attr.mode = 0444;
> + sahara_data->ddr_attr.read = ddr_training_read;
> +
> + /* Size is populated during device bootup */
Where? In some other patch?
> + sahara_data->ddr_attr.size = 0;
> + sahara_data->ddr_attr.write = NULL;
> +
> + /*
> + * Remove any existing sysfs binary attribute to avoid stale entries
> + * during warm boot or reinitialization. This ensures clean state before
> + * re-creating the attribute.
But why do you need to recreate it? What is the life cycle of this file
and how does it conflict with the life cycle of the sahara MHI device?
> + */
> + device_remove_bin_file(&context->mhi_dev->mhi_cntrl->mhi_dev->dev,
> + &sahara_data->ddr_attr);
> +
> + /* Create the binary attribute */
> + ret = device_create_bin_file(&context->mhi_dev->mhi_cntrl->mhi_dev->dev,
> + &sahara_data->ddr_attr);
> +
> + return ret;
> +}
> +
> static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_id *id)
> {
> struct sahara_context *context;
> int ret;
> int i;
> + struct sahara_dev_driver_data *sahara_data;
> + struct sahara_dev_trng_data *sdev;
This had a nice reverse xmas tree feel to it...perhaps you can keep
that?
>
> context = devm_kzalloc(&mhi_dev->dev, sizeof(*context), GFP_KERNEL);
> if (!context)
> @@ -717,6 +806,17 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
> if (!context->rx)
> return -ENOMEM;
>
> + sahara_data = kzalloc(sizeof(*sahara_data), GFP_KERNEL);
For the case where userspace doesn't read the DDR training data (e.g.
because the particular device doesn't implement/need that), where is
this freed?
> + if (!sahara_data)
> + return -ENOMEM;
> +
> + sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
> + if (!sdev)
> + return -ENOMEM;
> +
> + sahara_data->context = context;
> + sahara_data->sdev = sdev;
> +
> /*
> * AIC100 defines SAHARA_TRANSFER_MAX_SIZE as the largest value it
> * will request for READ_DATA. This is larger than
> @@ -749,7 +849,14 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
> return -EINVAL;
>
> context->active_image_id = SAHARA_IMAGE_ID_NONE;
> - dev_set_drvdata(&mhi_dev->dev, context);
> + dev_set_drvdata(&mhi_dev->dev, sahara_data);
sahara_mhi_remove and sahara_mhi_dl_xfer_cb still assumes that drvdata
is of type struct sahara_context.
Regards,
Bjorn
> +
> + ret = sahara_dev_populate_attribute(sahara_data);
> + if (ret) {
> + dev_err(&context->mhi_dev->dev,
> + "Failed to populate bin attribute: %d\n", ret);
> + return ret;
> + }
>
> ret = mhi_prepare_for_transfer(mhi_dev);
> if (ret)
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 08/12] drivers: soc: qcom: Support Sahara command mode packets(READY and EXECUTE)
2025-08-25 10:19 [PATCH v1 00/12] Sahara protocol enhancements Kishore Batta
` (6 preceding siblings ...)
2025-08-25 10:19 ` [PATCH v1 07/12] drivers: soc: qcom: Add sysfs support for DDR training data in Sahara Kishore Batta
@ 2025-08-25 10:19 ` Kishore Batta
2025-08-25 22:58 ` Bjorn Andersson
2025-08-28 14:20 ` Krzysztof Kozlowski
2025-08-25 10:19 ` [PATCH v1 09/12] drivers: soc: qcom: Remove is_mem_dump_mode variable Kishore Batta
` (3 subsequent siblings)
11 siblings, 2 replies; 26+ messages in thread
From: Kishore Batta @ 2025-08-25 10:19 UTC (permalink / raw)
To: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio
Cc: andersson, linux-arm-msm
During device boot, the device performs DDR training, and this training
data needs to be stored at the host end to improve subsequent boot times.
The Sahara protocol specification indicates that DDR training data can
be stored at the host end using command mode packets. The device sends
the command mode packet to the host through the HELLO packet, and the
host changes its mode of operation accordingly.
Once the device determines that it needs to change the mode of operation
to command mode, it sends the command ready packet. The host receives
the command ready packet and then sends command 8 to get the list of
commands supported by the device as per Sahara protocol specification.
Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
---
drivers/soc/qcom/sahara/sahara.c | 91 +++++++++++++++++++++++++++++---
1 file changed, 85 insertions(+), 6 deletions(-)
diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
index df103473af3a..84327af48569 100644
--- a/drivers/soc/qcom/sahara/sahara.c
+++ b/drivers/soc/qcom/sahara/sahara.c
@@ -59,6 +59,9 @@
#define SAHARA_RESET_LENGTH 0x8
#define SAHARA_MEM_DEBUG64_LENGTH 0x18
#define SAHARA_MEM_READ64_LENGTH 0x18
+#define SAHARA_COMMAND_READY_LENGTH 0x8
+#define SAHARA_COMMAND_EXECUTE_LENGTH 0xc
+#define SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST 0x8
struct sahara_dev_trng_data {
void *trng_data;
@@ -66,6 +69,13 @@ struct sahara_dev_trng_data {
bool receiving_trng_data;
};
+enum sahara_mode {
+ SAHARA_MODE_NONE,
+ SAHARA_MODE_MEM_DUMP,
+ SAHARA_MODE_CMD,
+ SAHARA_MODE_FW_DOWNLOAD,
+};
+
struct sahara_packet {
__le32 cmd;
__le32 length;
@@ -100,6 +110,9 @@ struct sahara_packet {
__le64 memory_address;
__le64 memory_length;
} memory_read64;
+ struct {
+ __le32 client_command;
+ } command_execute;
};
};
@@ -181,6 +194,7 @@ struct sahara_context {
void *mem_dump_freespace;
u64 dump_images_left;
bool is_mem_dump_mode;
+ enum sahara_mode current_mode;
};
struct sahara_dev_driver_data {
@@ -282,8 +296,15 @@ static void sahara_release_image(struct sahara_context *context)
static void sahara_send_reset(struct sahara_context *context)
{
int ret;
+ struct sahara_dev_driver_data *sahara_data;
+ struct sahara_dev_trng_data *sdev;
+
+ sahara_data = dev_get_drvdata(&context->mhi_dev->dev);
+ sdev = sahara_data->sdev;
context->is_mem_dump_mode = false;
+ context->current_mode = SAHARA_MODE_NONE;
+ sdev->receiving_trng_data = false;
context->tx[0]->cmd = cpu_to_le32(SAHARA_RESET_CMD);
context->tx[0]->length = cpu_to_le32(SAHARA_RESET_LENGTH);
@@ -297,6 +318,7 @@ static void sahara_send_reset(struct sahara_context *context)
static void sahara_hello(struct sahara_context *context)
{
int ret;
+ u32 mode;
dev_dbg(&context->mhi_dev->dev,
"HELLO cmd received. length:%d version:%d version_compat:%d max_length:%d mode:%d\n",
@@ -317,11 +339,17 @@ static void sahara_hello(struct sahara_context *context)
return;
}
- if (le32_to_cpu(context->rx->hello.mode) != SAHARA_MODE_IMAGE_TX_PENDING &&
- le32_to_cpu(context->rx->hello.mode) != SAHARA_MODE_IMAGE_TX_COMPLETE &&
- le32_to_cpu(context->rx->hello.mode) != SAHARA_MODE_MEMORY_DEBUG) {
+ mode = le32_to_cpu(context->rx->hello.mode);
+
+ switch (mode) {
+ case SAHARA_MODE_IMAGE_TX_PENDING:
+ case SAHARA_MODE_IMAGE_TX_COMPLETE:
+ case SAHARA_MODE_MEMORY_DEBUG:
+ case SAHARA_MODE_COMMAND:
+ break;
+ default:
dev_err(&context->mhi_dev->dev, "Unsupported hello packet - mode %d\n",
- le32_to_cpu(context->rx->hello.mode));
+ mode);
return;
}
@@ -514,6 +542,46 @@ static void sahara_memory_debug64(struct sahara_context *context)
dev_err(&context->mhi_dev->dev, "Unable to send read for dump table %d\n", ret);
}
+static void sahara_command_execute(struct sahara_context *context, u32 client_command)
+{
+ int ret;
+
+ context->tx[0]->cmd = cpu_to_le32(SAHARA_EXECUTE_CMD);
+ context->tx[0]->length = cpu_to_le32(SAHARA_COMMAND_EXECUTE_LENGTH);
+ context->tx[0]->command_execute.client_command = client_command;
+
+ ret = mhi_queue_buf(context->mhi_dev, DMA_TO_DEVICE, context->tx[0],
+ SAHARA_COMMAND_EXECUTE_LENGTH, MHI_EOT);
+
+ if (ret)
+ dev_err(&context->mhi_dev->dev, "Unable to send command execute %d\n", ret);
+}
+
+static void sahara_command_ready(struct sahara_context *context)
+{
+ dev_dbg(&context->mhi_dev->dev,
+ "Command ready cmd received. Length:%d\n",
+ le32_to_cpu(context->rx->length));
+
+ if (le32_to_cpu(context->rx->length) != SAHARA_COMMAND_READY_LENGTH) {
+ dev_err(&context->mhi_dev->dev, "Malformed command ready packet - length %d\n",
+ le32_to_cpu(context->rx->length));
+ return;
+ }
+
+ /*
+ * If the device sends the command ready packet, then its an indication
+ * to host that it needs to switch to command mode.
+ */
+ context->current_mode = SAHARA_MODE_CMD;
+
+ /*
+ * As per sahara spec, the host needs to send command ID 8 to get the
+ * list of commands supported by the device.
+ */
+ sahara_command_execute(context, SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST);
+}
+
static void sahara_processing(struct work_struct *work)
{
struct sahara_context *context = container_of(work, struct sahara_context, fw_work);
@@ -538,6 +606,9 @@ static void sahara_processing(struct work_struct *work)
case SAHARA_MEM_DEBUG64_CMD:
sahara_memory_debug64(context);
break;
+ case SAHARA_CMD_READY_CMD:
+ sahara_command_ready(context);
+ break;
default:
dev_err(&context->mhi_dev->dev, "Unknown command %d\n",
le32_to_cpu(context->rx->cmd));
@@ -873,7 +944,11 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
static void sahara_mhi_remove(struct mhi_device *mhi_dev)
{
- struct sahara_context *context = dev_get_drvdata(&mhi_dev->dev);
+ struct sahara_dev_driver_data *sahara_data;
+ struct sahara_context *context;
+
+ sahara_data = dev_get_drvdata(&mhi_dev->dev);
+ context = sahara_data->context;
cancel_work_sync(&context->fw_work);
cancel_work_sync(&context->dump_work);
@@ -888,7 +963,11 @@ static void sahara_mhi_ul_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result
static void sahara_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result *mhi_result)
{
- struct sahara_context *context = dev_get_drvdata(&mhi_dev->dev);
+ struct sahara_dev_driver_data *sahara_data;
+ struct sahara_context *context;
+
+ sahara_data = dev_get_drvdata(&mhi_dev->dev);
+ context = sahara_data->context;
if (!mhi_result->transaction_status) {
context->rx_size = mhi_result->bytes_xferd;
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v1 08/12] drivers: soc: qcom: Support Sahara command mode packets(READY and EXECUTE)
2025-08-25 10:19 ` [PATCH v1 08/12] drivers: soc: qcom: Support Sahara command mode packets(READY and EXECUTE) Kishore Batta
@ 2025-08-25 22:58 ` Bjorn Andersson
2025-08-28 14:20 ` Krzysztof Kozlowski
1 sibling, 0 replies; 26+ messages in thread
From: Bjorn Andersson @ 2025-08-25 22:58 UTC (permalink / raw)
To: Kishore Batta
Cc: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio,
linux-arm-msm
On Mon, Aug 25, 2025 at 03:49:22PM +0530, Kishore Batta wrote:
> During device boot, the device performs DDR training, and this training
> data needs to be stored at the host end to improve subsequent boot times.
As with the previous patch, I'd like to see this to be clarified. All
devices? Some devices? The tail end of the sentence indicate that this
is a performance improvement, is it?
Describe when DDR training happens, how it relates to Sahara and what
support a device that implements this needs from the host.
Note also, that above and below text does not belong in the same
paragraph, they are talking about different things.
> The Sahara protocol specification indicates that DDR training data can
> be stored at the host end using command mode packets. The device sends
> the command mode packet to the host through the HELLO packet, and the
> host changes its mode of operation accordingly.
>
Okay, so the HELLO packet contains the information about the mode...
> Once the device determines that it needs to change the mode of operation
> to command mode, it sends the command ready packet.
...but what does this then describe?
> The host receives
> the command ready packet and then sends command 8 to get the list of
> commands supported by the device as per Sahara protocol specification.
>
And then what?
Imagine that the reader doesn't know how the DDR training exchange over
Sahara works when they start reading this, will they have developed that
understanding when they get to the end of this text?
> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
> ---
> drivers/soc/qcom/sahara/sahara.c | 91 +++++++++++++++++++++++++++++---
> 1 file changed, 85 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
> index df103473af3a..84327af48569 100644
> --- a/drivers/soc/qcom/sahara/sahara.c
> +++ b/drivers/soc/qcom/sahara/sahara.c
> @@ -59,6 +59,9 @@
> #define SAHARA_RESET_LENGTH 0x8
> #define SAHARA_MEM_DEBUG64_LENGTH 0x18
> #define SAHARA_MEM_READ64_LENGTH 0x18
> +#define SAHARA_COMMAND_READY_LENGTH 0x8
> +#define SAHARA_COMMAND_EXECUTE_LENGTH 0xc
> +#define SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST 0x8
>
> struct sahara_dev_trng_data {
> void *trng_data;
> @@ -66,6 +69,13 @@ struct sahara_dev_trng_data {
> bool receiving_trng_data;
> };
>
> +enum sahara_mode {
> + SAHARA_MODE_NONE,
> + SAHARA_MODE_MEM_DUMP,
> + SAHARA_MODE_CMD,
> + SAHARA_MODE_FW_DOWNLOAD,
> +};
> +
> struct sahara_packet {
> __le32 cmd;
> __le32 length;
> @@ -100,6 +110,9 @@ struct sahara_packet {
> __le64 memory_address;
> __le64 memory_length;
> } memory_read64;
> + struct {
> + __le32 client_command;
> + } command_execute;
> };
> };
>
> @@ -181,6 +194,7 @@ struct sahara_context {
> void *mem_dump_freespace;
> u64 dump_images_left;
> bool is_mem_dump_mode;
> + enum sahara_mode current_mode;
> };
>
> struct sahara_dev_driver_data {
> @@ -282,8 +296,15 @@ static void sahara_release_image(struct sahara_context *context)
> static void sahara_send_reset(struct sahara_context *context)
> {
> int ret;
> + struct sahara_dev_driver_data *sahara_data;
> + struct sahara_dev_trng_data *sdev;
> +
> + sahara_data = dev_get_drvdata(&context->mhi_dev->dev);
> + sdev = sahara_data->sdev;
>
> context->is_mem_dump_mode = false;
> + context->current_mode = SAHARA_MODE_NONE;
> + sdev->receiving_trng_data = false;
This is never set to true. So yet another incomplete patch?
>
> context->tx[0]->cmd = cpu_to_le32(SAHARA_RESET_CMD);
> context->tx[0]->length = cpu_to_le32(SAHARA_RESET_LENGTH);
> @@ -297,6 +318,7 @@ static void sahara_send_reset(struct sahara_context *context)
> static void sahara_hello(struct sahara_context *context)
> {
> int ret;
> + u32 mode;
>
> dev_dbg(&context->mhi_dev->dev,
> "HELLO cmd received. length:%d version:%d version_compat:%d max_length:%d mode:%d\n",
> @@ -317,11 +339,17 @@ static void sahara_hello(struct sahara_context *context)
> return;
> }
>
> - if (le32_to_cpu(context->rx->hello.mode) != SAHARA_MODE_IMAGE_TX_PENDING &&
> - le32_to_cpu(context->rx->hello.mode) != SAHARA_MODE_IMAGE_TX_COMPLETE &&
> - le32_to_cpu(context->rx->hello.mode) != SAHARA_MODE_MEMORY_DEBUG) {
> + mode = le32_to_cpu(context->rx->hello.mode);
> +
> + switch (mode) {
> + case SAHARA_MODE_IMAGE_TX_PENDING:
> + case SAHARA_MODE_IMAGE_TX_COMPLETE:
> + case SAHARA_MODE_MEMORY_DEBUG:
> + case SAHARA_MODE_COMMAND:
You're effectively adding one more condition to the if statement. One
can argue if the if statement or the switch is the cleaner way to write
that, but when you replace 4 lines and add 11 it's hard to see that all
you did was add one more accepted mode.
> + break;
> + default:
> dev_err(&context->mhi_dev->dev, "Unsupported hello packet - mode %d\n",
> - le32_to_cpu(context->rx->hello.mode));
> + mode);
> return;
> }
>
> @@ -514,6 +542,46 @@ static void sahara_memory_debug64(struct sahara_context *context)
> dev_err(&context->mhi_dev->dev, "Unable to send read for dump table %d\n", ret);
> }
>
> +static void sahara_command_execute(struct sahara_context *context, u32 client_command)
> +{
> + int ret;
> +
> + context->tx[0]->cmd = cpu_to_le32(SAHARA_EXECUTE_CMD);
> + context->tx[0]->length = cpu_to_le32(SAHARA_COMMAND_EXECUTE_LENGTH);
> + context->tx[0]->command_execute.client_command = client_command;
> +
> + ret = mhi_queue_buf(context->mhi_dev, DMA_TO_DEVICE, context->tx[0],
> + SAHARA_COMMAND_EXECUTE_LENGTH, MHI_EOT);
> +
> + if (ret)
> + dev_err(&context->mhi_dev->dev, "Unable to send command execute %d\n", ret);
> +}
> +
> +static void sahara_command_ready(struct sahara_context *context)
> +{
> + dev_dbg(&context->mhi_dev->dev,
> + "Command ready cmd received. Length:%d\n",
> + le32_to_cpu(context->rx->length));
> +
> + if (le32_to_cpu(context->rx->length) != SAHARA_COMMAND_READY_LENGTH) {
> + dev_err(&context->mhi_dev->dev, "Malformed command ready packet - length %d\n",
> + le32_to_cpu(context->rx->length));
> + return;
> + }
> +
> + /*
> + * If the device sends the command ready packet, then its an indication
"If the device sends" sounds conditional - but if you're here the device
did send a command ready packet. And it's not an "indication", it really
means that we're switching to command mode.
> + * to host that it needs to switch to command mode.
> + */
> + context->current_mode = SAHARA_MODE_CMD;
> +
> + /*
> + * As per sahara spec, the host needs to send command ID 8 to get the
> + * list of commands supported by the device.
> + */
> + sahara_command_execute(context, SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST);
What will the device send next? Where is this handled? In the next^2
patch?
> +}
> +
> static void sahara_processing(struct work_struct *work)
> {
> struct sahara_context *context = container_of(work, struct sahara_context, fw_work);
> @@ -538,6 +606,9 @@ static void sahara_processing(struct work_struct *work)
> case SAHARA_MEM_DEBUG64_CMD:
> sahara_memory_debug64(context);
> break;
> + case SAHARA_CMD_READY_CMD:
> + sahara_command_ready(context);
> + break;
> default:
> dev_err(&context->mhi_dev->dev, "Unknown command %d\n",
> le32_to_cpu(context->rx->cmd));
> @@ -873,7 +944,11 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
>
> static void sahara_mhi_remove(struct mhi_device *mhi_dev)
> {
> - struct sahara_context *context = dev_get_drvdata(&mhi_dev->dev);
> + struct sahara_dev_driver_data *sahara_data;
> + struct sahara_context *context;
> +
> + sahara_data = dev_get_drvdata(&mhi_dev->dev);
> + context = sahara_data->context;
>
> cancel_work_sync(&context->fw_work);
> cancel_work_sync(&context->dump_work);
> @@ -888,7 +963,11 @@ static void sahara_mhi_ul_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result
>
> static void sahara_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result *mhi_result)
> {
> - struct sahara_context *context = dev_get_drvdata(&mhi_dev->dev);
> + struct sahara_dev_driver_data *sahara_data;
> + struct sahara_context *context;
> +
> + sahara_data = dev_get_drvdata(&mhi_dev->dev);
> + context = sahara_data->context;
This was broken between the patches.
Make sure you structure your patches such that:
1) One can reason about the whole thing that the patch introduces
2) Don't break the system inbetween any two patches - as this prevents
the use of "git bisect"
Regards,
Bjorn
>
> if (!mhi_result->transaction_status) {
> context->rx_size = mhi_result->bytes_xferd;
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 08/12] drivers: soc: qcom: Support Sahara command mode packets(READY and EXECUTE)
2025-08-25 10:19 ` [PATCH v1 08/12] drivers: soc: qcom: Support Sahara command mode packets(READY and EXECUTE) Kishore Batta
2025-08-25 22:58 ` Bjorn Andersson
@ 2025-08-28 14:20 ` Krzysztof Kozlowski
1 sibling, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2025-08-28 14:20 UTC (permalink / raw)
To: Kishore Batta, jeff.hugo, bjorn.andersson, konradybcio,
konrad.dybcio
Cc: andersson, linux-arm-msm
On 25/08/2025 12:19, Kishore Batta wrote:
> During device boot, the device performs DDR training, and this training
> data needs to be stored at the host end to improve subsequent boot times.
> The Sahara protocol specification indicates that DDR training data can
> be stored at the host end using command mode packets. The device sends
> the command mode packet to the host through the HELLO packet, and the
> host changes its mode of operation accordingly.
>
> Once the device determines that it needs to change the mode of operation
> to command mode, it sends the command ready packet. The host receives
> the command ready packet and then sends command 8 to get the list of
> commands supported by the device as per Sahara protocol specification.
>
> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
> ---
All of your patches use incorrect subject prefixes.
Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 09/12] drivers: soc: qcom: Remove is_mem_dump_mode variable.
2025-08-25 10:19 [PATCH v1 00/12] Sahara protocol enhancements Kishore Batta
` (7 preceding siblings ...)
2025-08-25 10:19 ` [PATCH v1 08/12] drivers: soc: qcom: Support Sahara command mode packets(READY and EXECUTE) Kishore Batta
@ 2025-08-25 10:19 ` Kishore Batta
2025-08-25 22:59 ` Bjorn Andersson
2025-08-25 10:19 ` [PATCH v1 10/12] drivers: soc: qcom: Support for DDR training in Sahara Kishore Batta
` (2 subsequent siblings)
11 siblings, 1 reply; 26+ messages in thread
From: Kishore Batta @ 2025-08-25 10:19 UTC (permalink / raw)
To: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio
Cc: andersson, linux-arm-msm
Sahara driver now has "sahara_mode" enum defined. So, the variable
"is_mem_dump_mode" is redundant. The mode of sahara is set to
SAHARA_MODE_MEM_DUMP wherever required.
Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
---
drivers/soc/qcom/sahara/sahara.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
index 84327af48569..81d9b40d0f92 100644
--- a/drivers/soc/qcom/sahara/sahara.c
+++ b/drivers/soc/qcom/sahara/sahara.c
@@ -193,7 +193,6 @@ struct sahara_context {
u64 dump_image_offset;
void *mem_dump_freespace;
u64 dump_images_left;
- bool is_mem_dump_mode;
enum sahara_mode current_mode;
};
@@ -302,7 +301,6 @@ static void sahara_send_reset(struct sahara_context *context)
sahara_data = dev_get_drvdata(&context->mhi_dev->dev);
sdev = sahara_data->sdev;
- context->is_mem_dump_mode = false;
context->current_mode = SAHARA_MODE_NONE;
sdev->receiving_trng_data = false;
@@ -515,7 +513,7 @@ static void sahara_memory_debug64(struct sahara_context *context)
* of the dump are that we can consume.
*/
- context->is_mem_dump_mode = true;
+ context->current_mode = SAHARA_MODE_MEM_DUMP;
/*
* Assume that the table is smaller than our MTU so that we can read it
@@ -971,7 +969,7 @@ static void sahara_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result
if (!mhi_result->transaction_status) {
context->rx_size = mhi_result->bytes_xferd;
- if (context->is_mem_dump_mode)
+ if (context->current_mode == SAHARA_MODE_MEM_DUMP)
schedule_work(&context->dump_work);
else
schedule_work(&context->fw_work);
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v1 09/12] drivers: soc: qcom: Remove is_mem_dump_mode variable.
2025-08-25 10:19 ` [PATCH v1 09/12] drivers: soc: qcom: Remove is_mem_dump_mode variable Kishore Batta
@ 2025-08-25 22:59 ` Bjorn Andersson
0 siblings, 0 replies; 26+ messages in thread
From: Bjorn Andersson @ 2025-08-25 22:59 UTC (permalink / raw)
To: Kishore Batta
Cc: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio,
linux-arm-msm
On Mon, Aug 25, 2025 at 03:49:23PM +0530, Kishore Batta wrote:
> Sahara driver now has "sahara_mode" enum defined. So, the variable
> "is_mem_dump_mode" is redundant. The mode of sahara is set to
> SAHARA_MODE_MEM_DUMP wherever required.
>
> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
> ---
> drivers/soc/qcom/sahara/sahara.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
> index 84327af48569..81d9b40d0f92 100644
> --- a/drivers/soc/qcom/sahara/sahara.c
> +++ b/drivers/soc/qcom/sahara/sahara.c
> @@ -193,7 +193,6 @@ struct sahara_context {
> u64 dump_image_offset;
> void *mem_dump_freespace;
> u64 dump_images_left;
> - bool is_mem_dump_mode;
> enum sahara_mode current_mode;
> };
>
> @@ -302,7 +301,6 @@ static void sahara_send_reset(struct sahara_context *context)
> sahara_data = dev_get_drvdata(&context->mhi_dev->dev);
> sdev = sahara_data->sdev;
>
> - context->is_mem_dump_mode = false;
> context->current_mode = SAHARA_MODE_NONE;
> sdev->receiving_trng_data = false;
>
> @@ -515,7 +513,7 @@ static void sahara_memory_debug64(struct sahara_context *context)
> * of the dump are that we can consume.
> */
>
> - context->is_mem_dump_mode = true;
> + context->current_mode = SAHARA_MODE_MEM_DUMP;
>
> /*
> * Assume that the table is smaller than our MTU so that we can read it
> @@ -971,7 +969,7 @@ static void sahara_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result
>
> if (!mhi_result->transaction_status) {
> context->rx_size = mhi_result->bytes_xferd;
> - if (context->is_mem_dump_mode)
> + if (context->current_mode == SAHARA_MODE_MEM_DUMP)
So for a moment you had two different ways to keep track of the
"mem_dump" state. If you reorder your patches to first replace
is_mem_dump_mode with the enum based mechanism and then add the
additional state this would be cleaner.
Regards,
Bjorn
> schedule_work(&context->dump_work);
> else
> schedule_work(&context->fw_work);
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 10/12] drivers: soc: qcom: Support for DDR training in Sahara.
2025-08-25 10:19 [PATCH v1 00/12] Sahara protocol enhancements Kishore Batta
` (8 preceding siblings ...)
2025-08-25 10:19 ` [PATCH v1 09/12] drivers: soc: qcom: Remove is_mem_dump_mode variable Kishore Batta
@ 2025-08-25 10:19 ` Kishore Batta
2025-08-25 23:14 ` Bjorn Andersson
2025-08-25 10:19 ` [PATCH v1 11/12] drivers: soc: qcom: Support to load saved DDR training data " Kishore Batta
2025-08-25 10:19 ` [PATCH v1 12/12] Add sysfs ABI documentation for DDR training data node Kishore Batta
11 siblings, 1 reply; 26+ messages in thread
From: Kishore Batta @ 2025-08-25 10:19 UTC (permalink / raw)
To: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio
Cc: andersson, linux-arm-msm
Support DDR training using Sahara command mode packets. Once the COMMAND
EXECUTE packet is sent to the device, it responds with a packet containing
the command ID. At this stage of boot, only the DDR training data command
(ID - 9) is supported by the device. The host sends the command execute
packet to execute command 9, and the device responds with the training
data. This data is the actual DDR training data attached to the device,
and its size is included in the response packet sent by the device.
Based on the size of the training data, the host queues multiple buffers
to MHI to receive the data. This data is stored in a memory location
and made available to the sysfs node.
Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
---
drivers/soc/qcom/sahara/sahara.c | 197 +++++++++++++++++++++++++++++++
1 file changed, 197 insertions(+)
diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
index 81d9b40d0f92..3887cdcfe256 100644
--- a/drivers/soc/qcom/sahara/sahara.c
+++ b/drivers/soc/qcom/sahara/sahara.c
@@ -60,8 +60,14 @@
#define SAHARA_MEM_DEBUG64_LENGTH 0x18
#define SAHARA_MEM_READ64_LENGTH 0x18
#define SAHARA_COMMAND_READY_LENGTH 0x8
+#define SAHARA_COMMAND_EXEC_RESP_LENGTH 0x10
#define SAHARA_COMMAND_EXECUTE_LENGTH 0xc
+#define SAHARA_COMMAND_EXEC_DATA_LENGTH 0xc
+#define SAHARA_SWITCH_MODE_LENGTH 0xc
+
#define SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST 0x8
+#define SAHARA_EXEC_CMD_GET_TRAINING_DATA 0x9
+#define SAHARA_NUM_CMD_BUF SAHARA_NUM_TX_BUF
struct sahara_dev_trng_data {
void *trng_data;
@@ -113,6 +119,16 @@ struct sahara_packet {
struct {
__le32 client_command;
} command_execute;
+ struct {
+ __le32 client_command;
+ __le32 response_length;
+ } command_execute_resp;
+ struct {
+ __le32 client_command;
+ } command_exec_data;
+ struct {
+ __le32 mode;
+ } mode_switch;
};
};
@@ -178,6 +194,7 @@ struct sahara_context {
struct sahara_packet *rx;
struct work_struct fw_work;
struct work_struct dump_work;
+ struct work_struct cmd_work;
struct mhi_device *mhi_dev;
const char * const *image_table;
u32 table_size;
@@ -194,6 +211,8 @@ struct sahara_context {
void *mem_dump_freespace;
u64 dump_images_left;
enum sahara_mode current_mode;
+ char *cmd_buff[SAHARA_NUM_CMD_BUF];
+ u64 bytes_copied;
};
struct sahara_dev_driver_data {
@@ -555,6 +574,21 @@ static void sahara_command_execute(struct sahara_context *context, u32 client_co
dev_err(&context->mhi_dev->dev, "Unable to send command execute %d\n", ret);
}
+static void sahara_switch_mode_to_img_tx(struct sahara_context *context)
+{
+ int ret;
+
+ context->tx[0]->cmd = cpu_to_le32(SAHARA_SWITCH_MODE_CMD);
+ context->tx[0]->length = cpu_to_le32(SAHARA_SWITCH_MODE_LENGTH);
+ context->tx[0]->mode_switch.mode = SAHARA_MODE_IMAGE_TX_PENDING;
+
+ ret = mhi_queue_buf(context->mhi_dev, DMA_TO_DEVICE, context->tx[0],
+ SAHARA_SWITCH_MODE_LENGTH, MHI_EOT);
+
+ if (ret)
+ dev_err(&context->mhi_dev->dev, "Unable to send mode switch %d\n", ret);
+}
+
static void sahara_command_ready(struct sahara_context *context)
{
dev_dbg(&context->mhi_dev->dev,
@@ -580,6 +614,165 @@ static void sahara_command_ready(struct sahara_context *context)
sahara_command_execute(context, SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST);
}
+static void sahara_alloc_mem_training_data(struct sahara_context *context)
+{
+ struct sahara_dev_driver_data *sahara_data;
+ struct sahara_dev_trng_data *sdev;
+
+ sahara_data = dev_get_drvdata(&context->mhi_dev->dev);
+ sdev = sahara_data->sdev;
+
+ sdev->trng_data = kzalloc(sdev->trng_data_sz, GFP_KERNEL);
+ if (!sdev->trng_data) {
+ sahara_send_reset(context);
+ return;
+ }
+
+ for (int i = 0; i < SAHARA_NUM_CMD_BUF; ++i) {
+ context->cmd_buff[i] = devm_kzalloc(&context->mhi_dev->dev,
+ SAHARA_PACKET_MAX_SIZE,
+ GFP_KERNEL);
+ if (!context->cmd_buff[i]) {
+ dev_err(&context->mhi_dev->dev,
+ "Failed to allocate cmd_buff[%d]\n", i);
+ sahara_send_reset(context);
+ /*
+ * Mark unused entries as NULL to avoid accidental usage
+ */
+ while (--i >= 0)
+ context->cmd_buff[i] = NULL;
+
+ return;
+ }
+ }
+}
+
+static void sahara_command_execute_resp(struct sahara_context *context)
+{
+ int ret;
+ int client_command;
+ int response_length;
+ struct sahara_dev_driver_data *sahara_data = dev_get_drvdata(&context->mhi_dev->dev);
+ struct sahara_dev_trng_data *sdev = sahara_data->sdev;
+
+ dev_dbg(&context->mhi_dev->dev,
+ "Command execute resp received. Length: %d resp_length: %d\n",
+ le32_to_cpu(context->rx->length),
+ le32_to_cpu(context->rx->command_execute_resp.response_length));
+
+ if (le32_to_cpu(context->rx->length) != SAHARA_COMMAND_EXEC_RESP_LENGTH ||
+ le32_to_cpu(context->rx->command_execute_resp.response_length < 0)) {
+ dev_err(&context->mhi_dev->dev,
+ "Malformed command execute resp packet - length %d\n",
+ le32_to_cpu(context->rx->length));
+
+ return;
+ }
+
+ client_command = le32_to_cpu(context->rx->command_execute_resp.client_command);
+ response_length = le32_to_cpu(context->rx->command_execute_resp.response_length);
+
+ if (client_command == SAHARA_EXEC_CMD_GET_TRAINING_DATA) {
+ sdev->trng_data_sz = response_length;
+ sahara_data->ddr_attr.size = response_length;
+ sdev->receiving_trng_data = true;
+
+ sahara_alloc_mem_training_data(context);
+
+ /* Queue multiple buffers for receiving data */
+ u64 data_len = sdev->trng_data_sz;
+ u64 pkt_data_len;
+ int i;
+
+ for (i = 0; i < SAHARA_NUM_CMD_BUF && data_len; ++i) {
+ pkt_data_len = min(data_len, SAHARA_PACKET_MAX_SIZE);
+ ret = mhi_queue_buf(context->mhi_dev, DMA_FROM_DEVICE,
+ context->cmd_buff[i], pkt_data_len,
+ data_len <= pkt_data_len ? MHI_EOT : MHI_CHAIN);
+
+ if (ret) {
+ dev_err(&context->mhi_dev->dev,
+ "Unable to queue command buff %d\n", ret);
+ return;
+ }
+
+ data_len -= pkt_data_len;
+ }
+ }
+
+ context->tx[0]->cmd = cpu_to_le32(SAHARA_EXECUTE_DATA_CMD);
+ context->tx[0]->length = cpu_to_le32(SAHARA_COMMAND_EXEC_DATA_LENGTH);
+ context->tx[0]->command_exec_data.client_command =
+ cpu_to_le32(context->rx->command_execute_resp.client_command);
+
+ ret = mhi_queue_buf(context->mhi_dev, DMA_TO_DEVICE,
+ context->tx[0], SAHARA_COMMAND_EXEC_DATA_LENGTH, MHI_EOT);
+
+ if (ret)
+ dev_err(&context->mhi_dev->dev,
+ "Unable to send command exec get data command %d\n", ret);
+}
+
+static void sahara_handle_training_data(struct sahara_context *context)
+{
+ u64 bytes_copied = context->bytes_copied;
+ u64 bytes_to_copy;
+ int i;
+ struct sahara_dev_driver_data *sahara_data = dev_get_drvdata(&context->mhi_dev->dev);
+ struct sahara_dev_trng_data *sdev = sahara_data->sdev;
+
+ for (i = 0; i < SAHARA_NUM_CMD_BUF && bytes_copied < sdev->trng_data_sz; ++i) {
+ bytes_to_copy = min_t(size_t, SAHARA_PACKET_MAX_SIZE,
+ sdev->trng_data_sz - bytes_copied);
+
+ /* Copy the received data into the training data buffer */
+ memcpy(sdev->trng_data + bytes_copied, context->cmd_buff[i], bytes_to_copy);
+
+ bytes_copied += bytes_to_copy;
+ context->bytes_copied = bytes_copied;
+ }
+
+ if (bytes_copied == sdev->trng_data_sz) {
+ /* Handle completion of data reception */
+ sahara_switch_mode_to_img_tx(context);
+ context->current_mode = SAHARA_MODE_FW_DOWNLOAD;
+ sdev->receiving_trng_data = false;
+ }
+}
+
+static void sahara_command_processing(struct work_struct *work)
+{
+ struct sahara_context *context = container_of(work, struct sahara_context, cmd_work);
+ int ret;
+ struct sahara_dev_driver_data *sahara_data = dev_get_drvdata(&context->mhi_dev->dev);
+ struct sahara_dev_trng_data *sdev = sahara_data->sdev;
+
+ if (sdev->receiving_trng_data) {
+ sahara_handle_training_data(context);
+ } else {
+ switch (le32_to_cpu(context->rx->cmd)) {
+ case SAHARA_EXECUTE_RESP_CMD:
+ sahara_command_execute_resp(context);
+ break;
+ case SAHARA_EXEC_CMD_GET_TRAINING_DATA:
+ sahara_command_execute(context, SAHARA_EXEC_CMD_GET_TRAINING_DATA);
+ break;
+ default:
+ dev_err(&context->mhi_dev->dev,
+ "Invalid client command 0x%x\n", le32_to_cpu(context->rx->cmd));
+ break;
+ }
+ }
+
+ /* Requeue buffer for receiving next command */
+ if (!sdev->receiving_trng_data) {
+ ret = mhi_queue_buf(context->mhi_dev, DMA_FROM_DEVICE, context->rx,
+ SAHARA_PACKET_MAX_SIZE, MHI_EOT);
+ if (ret)
+ dev_err(&context->mhi_dev->dev, "Unable to requeue rx buf %d\n", ret);
+ }
+}
+
static void sahara_processing(struct work_struct *work)
{
struct sahara_context *context = container_of(work, struct sahara_context, fw_work);
@@ -906,6 +1099,7 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
context->mhi_dev = mhi_dev;
INIT_WORK(&context->fw_work, sahara_processing);
INIT_WORK(&context->dump_work, sahara_dump_processing);
+ INIT_WORK(&context->cmd_work, sahara_command_processing);
/* Get the image table for a given device name */
context->image_table = sahara_get_image_table(mhi_dev->mhi_cntrl->name);
@@ -950,6 +1144,7 @@ static void sahara_mhi_remove(struct mhi_device *mhi_dev)
cancel_work_sync(&context->fw_work);
cancel_work_sync(&context->dump_work);
+ cancel_work_sync(&context->cmd_work);
vfree(context->mem_dump);
sahara_release_image(context);
mhi_unprepare_from_transfer(mhi_dev);
@@ -971,6 +1166,8 @@ static void sahara_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result
context->rx_size = mhi_result->bytes_xferd;
if (context->current_mode == SAHARA_MODE_MEM_DUMP)
schedule_work(&context->dump_work);
+ else if (context->current_mode == SAHARA_MODE_CMD)
+ schedule_work(&context->cmd_work);
else
schedule_work(&context->fw_work);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v1 10/12] drivers: soc: qcom: Support for DDR training in Sahara.
2025-08-25 10:19 ` [PATCH v1 10/12] drivers: soc: qcom: Support for DDR training in Sahara Kishore Batta
@ 2025-08-25 23:14 ` Bjorn Andersson
0 siblings, 0 replies; 26+ messages in thread
From: Bjorn Andersson @ 2025-08-25 23:14 UTC (permalink / raw)
To: Kishore Batta
Cc: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio,
linux-arm-msm
On Mon, Aug 25, 2025 at 03:49:24PM +0530, Kishore Batta wrote:
> Support DDR training using Sahara command mode packets. Once the COMMAND
> EXECUTE packet is sent to the device, it responds with a packet containing
> the command ID. At this stage of boot, only the DDR training data command
> (ID - 9) is supported by the device. The host sends the command execute
> packet to execute command 9, and the device responds with the training
> data. This data is the actual DDR training data attached to the device,
> and its size is included in the response packet sent by the device.
>
> Based on the size of the training data, the host queues multiple buffers
> to MHI to receive the data. This data is stored in a memory location
> and made available to the sysfs node.
As I write at the tail end of this patch, this is scattered over a whole
bunch of patches now. The "made available to the sysfs node" only barely
relates to the content of this patch, but to HEAD^^^.
>
> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
> ---
> drivers/soc/qcom/sahara/sahara.c | 197 +++++++++++++++++++++++++++++++
> 1 file changed, 197 insertions(+)
>
> diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
> index 81d9b40d0f92..3887cdcfe256 100644
> --- a/drivers/soc/qcom/sahara/sahara.c
> +++ b/drivers/soc/qcom/sahara/sahara.c
> @@ -60,8 +60,14 @@
> #define SAHARA_MEM_DEBUG64_LENGTH 0x18
> #define SAHARA_MEM_READ64_LENGTH 0x18
> #define SAHARA_COMMAND_READY_LENGTH 0x8
> +#define SAHARA_COMMAND_EXEC_RESP_LENGTH 0x10
> #define SAHARA_COMMAND_EXECUTE_LENGTH 0xc
> +#define SAHARA_COMMAND_EXEC_DATA_LENGTH 0xc
> +#define SAHARA_SWITCH_MODE_LENGTH 0xc
> +
> #define SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST 0x8
> +#define SAHARA_EXEC_CMD_GET_TRAINING_DATA 0x9
> +#define SAHARA_NUM_CMD_BUF SAHARA_NUM_TX_BUF
>
> struct sahara_dev_trng_data {
> void *trng_data;
> @@ -113,6 +119,16 @@ struct sahara_packet {
> struct {
> __le32 client_command;
> } command_execute;
> + struct {
> + __le32 client_command;
> + __le32 response_length;
> + } command_execute_resp;
> + struct {
> + __le32 client_command;
> + } command_exec_data;
> + struct {
> + __le32 mode;
> + } mode_switch;
> };
> };
>
> @@ -178,6 +194,7 @@ struct sahara_context {
> struct sahara_packet *rx;
> struct work_struct fw_work;
> struct work_struct dump_work;
> + struct work_struct cmd_work;
> struct mhi_device *mhi_dev;
> const char * const *image_table;
> u32 table_size;
> @@ -194,6 +211,8 @@ struct sahara_context {
> void *mem_dump_freespace;
> u64 dump_images_left;
> enum sahara_mode current_mode;
> + char *cmd_buff[SAHARA_NUM_CMD_BUF];
> + u64 bytes_copied;
> };
>
> struct sahara_dev_driver_data {
> @@ -555,6 +574,21 @@ static void sahara_command_execute(struct sahara_context *context, u32 client_co
> dev_err(&context->mhi_dev->dev, "Unable to send command execute %d\n", ret);
> }
>
> +static void sahara_switch_mode_to_img_tx(struct sahara_context *context)
> +{
> + int ret;
> +
> + context->tx[0]->cmd = cpu_to_le32(SAHARA_SWITCH_MODE_CMD);
> + context->tx[0]->length = cpu_to_le32(SAHARA_SWITCH_MODE_LENGTH);
> + context->tx[0]->mode_switch.mode = SAHARA_MODE_IMAGE_TX_PENDING;
> +
> + ret = mhi_queue_buf(context->mhi_dev, DMA_TO_DEVICE, context->tx[0],
> + SAHARA_SWITCH_MODE_LENGTH, MHI_EOT);
> +
> + if (ret)
> + dev_err(&context->mhi_dev->dev, "Unable to send mode switch %d\n", ret);
> +}
> +
> static void sahara_command_ready(struct sahara_context *context)
> {
> dev_dbg(&context->mhi_dev->dev,
> @@ -580,6 +614,165 @@ static void sahara_command_ready(struct sahara_context *context)
> sahara_command_execute(context, SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST);
> }
>
> +static void sahara_alloc_mem_training_data(struct sahara_context *context)
> +{
> + struct sahara_dev_driver_data *sahara_data;
> + struct sahara_dev_trng_data *sdev;
> +
> + sahara_data = dev_get_drvdata(&context->mhi_dev->dev);
> + sdev = sahara_data->sdev;
> +
> + sdev->trng_data = kzalloc(sdev->trng_data_sz, GFP_KERNEL);
> + if (!sdev->trng_data) {
> + sahara_send_reset(context);
> + return;
> + }
> +
> + for (int i = 0; i < SAHARA_NUM_CMD_BUF; ++i) {
> + context->cmd_buff[i] = devm_kzalloc(&context->mhi_dev->dev,
> + SAHARA_PACKET_MAX_SIZE,
> + GFP_KERNEL);
Don't use devm allocations at runtime. When is this freed? When is the
sahara device reclaimed?
> + if (!context->cmd_buff[i]) {
> + dev_err(&context->mhi_dev->dev,
> + "Failed to allocate cmd_buff[%d]\n", i);
> + sahara_send_reset(context);
> + /*
> + * Mark unused entries as NULL to avoid accidental usage
> + */
> + while (--i >= 0)
> + context->cmd_buff[i] = NULL;
> +
> + return;
> + }
> + }
> +}
> +
> +static void sahara_command_execute_resp(struct sahara_context *context)
> +{
> + int ret;
> + int client_command;
> + int response_length;
> + struct sahara_dev_driver_data *sahara_data = dev_get_drvdata(&context->mhi_dev->dev);
> + struct sahara_dev_trng_data *sdev = sahara_data->sdev;
> +
> + dev_dbg(&context->mhi_dev->dev,
> + "Command execute resp received. Length: %d resp_length: %d\n",
> + le32_to_cpu(context->rx->length),
> + le32_to_cpu(context->rx->command_execute_resp.response_length));
> +
> + if (le32_to_cpu(context->rx->length) != SAHARA_COMMAND_EXEC_RESP_LENGTH ||
> + le32_to_cpu(context->rx->command_execute_resp.response_length < 0)) {
> + dev_err(&context->mhi_dev->dev,
> + "Malformed command execute resp packet - length %d\n",
> + le32_to_cpu(context->rx->length));
> +
> + return;
> + }
> +
> + client_command = le32_to_cpu(context->rx->command_execute_resp.client_command);
> + response_length = le32_to_cpu(context->rx->command_execute_resp.response_length);
> +
> + if (client_command == SAHARA_EXEC_CMD_GET_TRAINING_DATA) {
> + sdev->trng_data_sz = response_length;
> + sahara_data->ddr_attr.size = response_length;
> + sdev->receiving_trng_data = true;
> +
> + sahara_alloc_mem_training_data(context);
If either allocation in sahara_alloc_mem_training_data() fails you will
mhi_queue_buf NULL pointers below.
> +
> + /* Queue multiple buffers for receiving data */
> + u64 data_len = sdev->trng_data_sz;
> + u64 pkt_data_len;
> + int i;
> +
> + for (i = 0; i < SAHARA_NUM_CMD_BUF && data_len; ++i) {
> + pkt_data_len = min(data_len, SAHARA_PACKET_MAX_SIZE);
> + ret = mhi_queue_buf(context->mhi_dev, DMA_FROM_DEVICE,
> + context->cmd_buff[i], pkt_data_len,
> + data_len <= pkt_data_len ? MHI_EOT : MHI_CHAIN);
> +
> + if (ret) {
> + dev_err(&context->mhi_dev->dev,
> + "Unable to queue command buff %d\n", ret);
> + return;
> + }
> +
> + data_len -= pkt_data_len;
> + }
> + }
> +
> + context->tx[0]->cmd = cpu_to_le32(SAHARA_EXECUTE_DATA_CMD);
> + context->tx[0]->length = cpu_to_le32(SAHARA_COMMAND_EXEC_DATA_LENGTH);
> + context->tx[0]->command_exec_data.client_command =
> + cpu_to_le32(context->rx->command_execute_resp.client_command);
> +
> + ret = mhi_queue_buf(context->mhi_dev, DMA_TO_DEVICE,
> + context->tx[0], SAHARA_COMMAND_EXEC_DATA_LENGTH, MHI_EOT);
> +
> + if (ret)
> + dev_err(&context->mhi_dev->dev,
> + "Unable to send command exec get data command %d\n", ret);
> +}
> +
> +static void sahara_handle_training_data(struct sahara_context *context)
> +{
> + u64 bytes_copied = context->bytes_copied;
> + u64 bytes_to_copy;
> + int i;
> + struct sahara_dev_driver_data *sahara_data = dev_get_drvdata(&context->mhi_dev->dev);
> + struct sahara_dev_trng_data *sdev = sahara_data->sdev;
> +
> + for (i = 0; i < SAHARA_NUM_CMD_BUF && bytes_copied < sdev->trng_data_sz; ++i) {
> + bytes_to_copy = min_t(size_t, SAHARA_PACKET_MAX_SIZE,
> + sdev->trng_data_sz - bytes_copied);
> +
> + /* Copy the received data into the training data buffer */
> + memcpy(sdev->trng_data + bytes_copied, context->cmd_buff[i], bytes_to_copy);
> +
> + bytes_copied += bytes_to_copy;
> + context->bytes_copied = bytes_copied;
> + }
> +
> + if (bytes_copied == sdev->trng_data_sz) {
> + /* Handle completion of data reception */
> + sahara_switch_mode_to_img_tx(context);
> + context->current_mode = SAHARA_MODE_FW_DOWNLOAD;
> + sdev->receiving_trng_data = false;
> + }
> +}
> +
> +static void sahara_command_processing(struct work_struct *work)
> +{
> + struct sahara_context *context = container_of(work, struct sahara_context, cmd_work);
> + int ret;
> + struct sahara_dev_driver_data *sahara_data = dev_get_drvdata(&context->mhi_dev->dev);
> + struct sahara_dev_trng_data *sdev = sahara_data->sdev;
> +
> + if (sdev->receiving_trng_data) {
> + sahara_handle_training_data(context);
> + } else {
> + switch (le32_to_cpu(context->rx->cmd)) {
> + case SAHARA_EXECUTE_RESP_CMD:
> + sahara_command_execute_resp(context);
> + break;
> + case SAHARA_EXEC_CMD_GET_TRAINING_DATA:
> + sahara_command_execute(context, SAHARA_EXEC_CMD_GET_TRAINING_DATA);
> + break;
> + default:
> + dev_err(&context->mhi_dev->dev,
> + "Invalid client command 0x%x\n", le32_to_cpu(context->rx->cmd));
> + break;
> + }
> + }
> +
> + /* Requeue buffer for receiving next command */
> + if (!sdev->receiving_trng_data) {
> + ret = mhi_queue_buf(context->mhi_dev, DMA_FROM_DEVICE, context->rx,
> + SAHARA_PACKET_MAX_SIZE, MHI_EOT);
> + if (ret)
> + dev_err(&context->mhi_dev->dev, "Unable to requeue rx buf %d\n", ret);
> + }
> +}
> +
> static void sahara_processing(struct work_struct *work)
> {
> struct sahara_context *context = container_of(work, struct sahara_context, fw_work);
> @@ -906,6 +1099,7 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
> context->mhi_dev = mhi_dev;
> INIT_WORK(&context->fw_work, sahara_processing);
> INIT_WORK(&context->dump_work, sahara_dump_processing);
> + INIT_WORK(&context->cmd_work, sahara_command_processing);
>
> /* Get the image table for a given device name */
> context->image_table = sahara_get_image_table(mhi_dev->mhi_cntrl->name);
> @@ -950,6 +1144,7 @@ static void sahara_mhi_remove(struct mhi_device *mhi_dev)
>
> cancel_work_sync(&context->fw_work);
> cancel_work_sync(&context->dump_work);
> + cancel_work_sync(&context->cmd_work);
> vfree(context->mem_dump);
> sahara_release_image(context);
> mhi_unprepare_from_transfer(mhi_dev);
> @@ -971,6 +1166,8 @@ static void sahara_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result
> context->rx_size = mhi_result->bytes_xferd;
> if (context->current_mode == SAHARA_MODE_MEM_DUMP)
> schedule_work(&context->dump_work);
> + else if (context->current_mode == SAHARA_MODE_CMD)
> + schedule_work(&context->cmd_work);
That means that you introduced the command mode work spread across three
(four?) different patches, but the only way to reason about the addition
is to read them all and think of them as one change.
Please try to slice it differently - rather than introducing some piece
of command mode and then another piece, which at each step is
incomplete. This patch indicates that you get the list of commands and
you can choose to call them or not. So why not implement the command
list send, receive and handling in one patch - without calling any
command. Then another patch that implements the DDR training command.
Suddenly you have two (or more) patches that are coherent in themselves
- not scattered across multiple commits.
Regards,
Bjorn
> else
> schedule_work(&context->fw_work);
> }
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 11/12] drivers: soc: qcom: Support to load saved DDR training data in Sahara.
2025-08-25 10:19 [PATCH v1 00/12] Sahara protocol enhancements Kishore Batta
` (9 preceding siblings ...)
2025-08-25 10:19 ` [PATCH v1 10/12] drivers: soc: qcom: Support for DDR training in Sahara Kishore Batta
@ 2025-08-25 10:19 ` Kishore Batta
2025-08-26 2:16 ` Bjorn Andersson
2025-08-25 10:19 ` [PATCH v1 12/12] Add sysfs ABI documentation for DDR training data node Kishore Batta
11 siblings, 1 reply; 26+ messages in thread
From: Kishore Batta @ 2025-08-25 10:19 UTC (permalink / raw)
To: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio
Cc: andersson, linux-arm-msm
Load the saved DDR training data during subsequent bootups. The image ID 34
is for DDR training data image. During subsequent bootups, the Sahara
driver needs to load the training data file associated with the serial
number instead of the default file present in the image table. If the
serial number-specific file is not present in the firmware directory,
it indicates the first bootup of the device, and training data gets
generated.
Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
---
drivers/soc/qcom/sahara/sahara.c | 71 +++++++++++++++++++++++++++-----
1 file changed, 60 insertions(+), 11 deletions(-)
diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
index 3887cdcfe256..28e52a9974a1 100644
--- a/drivers/soc/qcom/sahara/sahara.c
+++ b/drivers/soc/qcom/sahara/sahara.c
@@ -68,6 +68,7 @@
#define SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST 0x8
#define SAHARA_EXEC_CMD_GET_TRAINING_DATA 0x9
#define SAHARA_NUM_CMD_BUF SAHARA_NUM_TX_BUF
+#define SAHARA_DDR_TRAINING_IMG_ID 34
struct sahara_dev_trng_data {
void *trng_data;
@@ -213,6 +214,8 @@ struct sahara_context {
enum sahara_mode current_mode;
char *cmd_buff[SAHARA_NUM_CMD_BUF];
u64 bytes_copied;
+ u32 serial_num;
+ char *fw_folder_name;
};
struct sahara_dev_driver_data {
@@ -270,6 +273,7 @@ static ssize_t ddr_training_read(struct file *filp, struct kobject *kobj,
static int sahara_find_image(struct sahara_context *context, u32 image_id)
{
int ret;
+ char *fw_path;
if (image_id == context->active_image_id)
return 0;
@@ -286,19 +290,64 @@ static int sahara_find_image(struct sahara_context *context, u32 image_id)
}
/*
- * This image might be optional. The device may continue without it.
- * Only the device knows. Suppress error messages that could suggest an
- * a problem when we were actually able to continue.
+ * Image ID 34 corresponds to DDR training data. On subsequent
+ * bootups, the sahara driver may need to load the training data file
+ * associated with device's serial number instead of the default file
+ * listed in the image table.
+ *
+ * If the serial number specific file is not found in the firmware
+ * directory, it likely indicates the device is booting for the first
+ * time, and new training data will be generated.
*/
- ret = firmware_request_nowarn(&context->firmware,
- context->image_table[image_id],
- &context->mhi_dev->dev);
- if (ret) {
- dev_dbg(&context->mhi_dev->dev, "request for image id %d / file %s failed %d\n",
- image_id, context->image_table[image_id], ret);
- return ret;
- }
+ if (image_id == SAHARA_DDR_TRAINING_IMG_ID) {
+ context->serial_num = context->mhi_dev->mhi_cntrl->serial_number;
+ context->fw_folder_name =
+ sahara_get_fw_folder_name(context->mhi_dev->mhi_cntrl->name);
+ if (!context->fw_folder_name)
+ return -EINVAL;
+
+ fw_path = devm_kasprintf(&context->mhi_dev->dev, GFP_KERNEL,
+ "qcom/%s/mdmddr_0x%x.mbn",
+ context->fw_folder_name,
+ context->serial_num);
+
+ if (!fw_path)
+ return -ENOMEM;
+
+ ret = firmware_request_nowarn(&context->firmware,
+ fw_path,
+ &context->mhi_dev->dev);
+
+ /*
+ * If there is failure to load serial number specific file, load
+ * the default file from image table
+ */
+ if (ret) {
+ ret = firmware_request_nowarn(&context->firmware,
+ context->image_table[image_id],
+ &context->mhi_dev->dev);
+ if (ret) {
+ dev_dbg(&context->mhi_dev->dev, "request for image id %d / file %s failed %d\n",
+ image_id, context->image_table[image_id], ret);
+ }
+ return ret;
+ }
+ } else {
+ /*
+ * This image might be optional. The device may continue without it.
+ * Only the device knows. Suppress error messages that could suggest an
+ * a problem when we were actually able to continue.
+ */
+ ret = firmware_request_nowarn(&context->firmware,
+ context->image_table[image_id],
+ &context->mhi_dev->dev);
+ if (ret) {
+ dev_dbg(&context->mhi_dev->dev, "request for image id %d / file %s failed %d\n",
+ image_id, context->image_table[image_id], ret);
+ return ret;
+ }
+ }
context->active_image_id = image_id;
return 0;
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v1 11/12] drivers: soc: qcom: Support to load saved DDR training data in Sahara.
2025-08-25 10:19 ` [PATCH v1 11/12] drivers: soc: qcom: Support to load saved DDR training data " Kishore Batta
@ 2025-08-26 2:16 ` Bjorn Andersson
0 siblings, 0 replies; 26+ messages in thread
From: Bjorn Andersson @ 2025-08-26 2:16 UTC (permalink / raw)
To: Kishore Batta
Cc: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio,
linux-arm-msm
On Mon, Aug 25, 2025 at 03:49:25PM +0530, Kishore Batta wrote:
> Load the saved DDR training data during subsequent bootups. The image ID 34
> is for DDR training data image. During subsequent bootups, the Sahara
> driver needs to load the training data file associated with the serial
> number instead of the default file present in the image table. If the
> serial number-specific file is not present in the firmware directory,
> it indicates the first bootup of the device, and training data gets
> generated.
As with your other commit messages, you start by stating what the patch
does and they you add layer after layer with explanation on top of that.
Rewrite this one as well to clearly describe what problem the patch is
trying to solve, why the firmware loader is invoked here and what
expectations this has on user space.
>
> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
> ---
> drivers/soc/qcom/sahara/sahara.c | 71 +++++++++++++++++++++++++++-----
> 1 file changed, 60 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
> index 3887cdcfe256..28e52a9974a1 100644
> --- a/drivers/soc/qcom/sahara/sahara.c
> +++ b/drivers/soc/qcom/sahara/sahara.c
> @@ -68,6 +68,7 @@
> #define SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST 0x8
> #define SAHARA_EXEC_CMD_GET_TRAINING_DATA 0x9
> #define SAHARA_NUM_CMD_BUF SAHARA_NUM_TX_BUF
> +#define SAHARA_DDR_TRAINING_IMG_ID 34
>
> struct sahara_dev_trng_data {
> void *trng_data;
> @@ -213,6 +214,8 @@ struct sahara_context {
> enum sahara_mode current_mode;
> char *cmd_buff[SAHARA_NUM_CMD_BUF];
> u64 bytes_copied;
> + u32 serial_num;
> + char *fw_folder_name;
Both of these are assigned and used solely within that one block below,
and they are 12 bytes. Can they not be put on the stack?
> };
>
> struct sahara_dev_driver_data {
> @@ -270,6 +273,7 @@ static ssize_t ddr_training_read(struct file *filp, struct kobject *kobj,
> static int sahara_find_image(struct sahara_context *context, u32 image_id)
> {
> int ret;
> + char *fw_path;
Reverse xmas-tree please.
>
> if (image_id == context->active_image_id)
> return 0;
> @@ -286,19 +290,64 @@ static int sahara_find_image(struct sahara_context *context, u32 image_id)
> }
>
> /*
> - * This image might be optional. The device may continue without it.
> - * Only the device knows. Suppress error messages that could suggest an
> - * a problem when we were actually able to continue.
> + * Image ID 34 corresponds to DDR training data. On subsequent
> + * bootups, the sahara driver may need to load the training data file
> + * associated with device's serial number instead of the default file
> + * listed in the image table.
> + *
> + * If the serial number specific file is not found in the firmware
> + * directory, it likely indicates the device is booting for the first
> + * time, and new training data will be generated.
> */
This comment only relates to the first block below. How about moving it
into the block to make that clear - and to keep symmetry with the other
comment that you moved into the its block?
> - ret = firmware_request_nowarn(&context->firmware,
> - context->image_table[image_id],
> - &context->mhi_dev->dev);
> - if (ret) {
> - dev_dbg(&context->mhi_dev->dev, "request for image id %d / file %s failed %d\n",
> - image_id, context->image_table[image_id], ret);
> - return ret;
> - }
> + if (image_id == SAHARA_DDR_TRAINING_IMG_ID) {
> + context->serial_num = context->mhi_dev->mhi_cntrl->serial_number;
> + context->fw_folder_name =
> + sahara_get_fw_folder_name(context->mhi_dev->mhi_cntrl->name);
Use a local variable for "name" to shorted the line, such that you don't
need to wrap it weirdly.
> + if (!context->fw_folder_name)
> + return -EINVAL;
> +
> + fw_path = devm_kasprintf(&context->mhi_dev->dev, GFP_KERNEL,
fw_path is used for 3 lines, but you're keeping this allocation for an
undefined amount of time.
Regards,
Bjorn
> + "qcom/%s/mdmddr_0x%x.mbn",
> + context->fw_folder_name,
> + context->serial_num);
> +
> + if (!fw_path)
> + return -ENOMEM;
> +
> + ret = firmware_request_nowarn(&context->firmware,
> + fw_path,
> + &context->mhi_dev->dev);
> +
> + /*
> + * If there is failure to load serial number specific file, load
> + * the default file from image table
> + */
> + if (ret) {
> + ret = firmware_request_nowarn(&context->firmware,
> + context->image_table[image_id],
> + &context->mhi_dev->dev);
> + if (ret) {
> + dev_dbg(&context->mhi_dev->dev, "request for image id %d / file %s failed %d\n",
> + image_id, context->image_table[image_id], ret);
> + }
> + return ret;
> + }
> + } else {
> + /*
> + * This image might be optional. The device may continue without it.
> + * Only the device knows. Suppress error messages that could suggest an
> + * a problem when we were actually able to continue.
> + */
> + ret = firmware_request_nowarn(&context->firmware,
> + context->image_table[image_id],
> + &context->mhi_dev->dev);
> + if (ret) {
> + dev_dbg(&context->mhi_dev->dev, "request for image id %d / file %s failed %d\n",
> + image_id, context->image_table[image_id], ret);
>
> + return ret;
> + }
> + }
> context->active_image_id = image_id;
>
> return 0;
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 12/12] Add sysfs ABI documentation for DDR training data node.
2025-08-25 10:19 [PATCH v1 00/12] Sahara protocol enhancements Kishore Batta
` (10 preceding siblings ...)
2025-08-25 10:19 ` [PATCH v1 11/12] drivers: soc: qcom: Support to load saved DDR training data " Kishore Batta
@ 2025-08-25 10:19 ` Kishore Batta
11 siblings, 0 replies; 26+ messages in thread
From: Kishore Batta @ 2025-08-25 10:19 UTC (permalink / raw)
To: jeff.hugo, bjorn.andersson, konradybcio, konrad.dybcio
Cc: andersson, linux-arm-msm
Add sysfs attribute documentation for the DDR training data node.
Userspace service reads the training data and then stores the card
specific data in the firmware image filesystem.
Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
---
.../testing/sysfs-bus-mhi-ddr_training_data | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-mhi-ddr_training_data
diff --git a/Documentation/ABI/testing/sysfs-bus-mhi-ddr_training_data b/Documentation/ABI/testing/sysfs-bus-mhi-ddr_training_data
new file mode 100644
index 000000000000..30fdcd383a01
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-mhi-ddr_training_data
@@ -0,0 +1,19 @@
+What: /sys/bus/mhi/devices/<mhi-cntrl>/ddr_training_data
+
+Date: August 2025
+
+Contact: Kishore Batta <kishore.batta@oss.qualcomm.com>
+
+Description: Contains the DDR training data for the Qualcomm device
+ connected. MHI driver populates different controller
+ nodes for each device. The DDR training data is exposed
+ to userspace to read and save the training data file to
+ the filesystem. In the subsequent boot up of the device,
+ the training data is restored from host to device
+ optimizing the boot up time of the device.
+
+Usage: Example for reading DDR training data:
+ cat /sys/bus/mhi/devices/mhi0/ddr_training_data
+
+Permissions: The file permissions are set to 0444 allowing read
+ access.
--
2.34.1
^ permalink raw reply related [flat|nested] 26+ messages in thread