[PATCH V4 0/2] mailbox: tmel-qmp: Introduce QCOM TMEL QMP mailbox driver

[PATCH V4 0/2] mailbox: tmel-qmp: Introduce QCOM TMEL QMP mailbox driver

[Sricharan R]

From: Sricharan Ramabadhran <quic_srichara@quicinc.com>

The QMP mailbox is the primary means of communication between TMEL
(Trust Management Engine Lite) SS and other subsystem on the SoC.
A dedicated pair of inbound and outbound mailboxes is implemented for
each subsystem/external execution environment which needs to communicate
with TMEL for security services. The inbound mailboxes are used to send
IPC requests to TME-L, which are then processed by TME-L firmware and
accordingly the responses are sent to the requestor via outbound
mailboxes.

It is an IPC transport protocol which is light weight and supports
a subset of API's. It handles link initialization, negotiation,
establishment and communication across client(APPSS/BTSS/AUDIOSS)
and server(TME-L SS).

   -----------------------------------------------       ---------------------------------------------------
  |                                              |       |                                                 |
  |                 SOC  CLIENT                  | SOC   |                TME-L  SS                        |
  |                                              | AHB   |                                                 |
  |     ----------    ---------   ---------      |       | ------    -------     --------    ------------  |
  |     |        |    |       |   |       |      | WO    | |     | R |     |     |      |    |SERVICES   | |
  |     | APPS   |<-->| TMEL  |<->|       |------------->| | IN  |-->|     |     | TMEL |    |--------   | |
  |     |        |    | COM   |   | QMP   |      | RO    | |     | W | QMP |<--->| COM  |<-->| a) ATTEST | |
  |     |        |    |       |   |       |<-------------| | OUT |<--|     |     |      |    | b) CRYPTO | |
  |     |        |    |       |   |       |      |       | |     |   |     |     |      |    | .. more   | |
  |     ---------     ---------   ---------      |       | ------    -------     -------     ------------  |
  |                                              |       |                                                 |
   -----------------------------------------------       --------------------------------------------------

TME-L SS provides different kinds of services like secureboot,
remote image authentication, key management, crypto, OEM provisioning etc.
This patch adds support for remote image authentication.
Support for rest of the services can be added.

Remote proc driver subscribes to this mailbox and uses the
mbox_send_message to use TME-L to securely authenticate/teardown the
images.

Since clients like same rproc driver use SCM/TMEL across socs, the goal
here was to abstract the TMEL-QMP SS functionality, so that clients should
be able to connect and send messages with a common API.

[V4]

        Fixed TME-L naming in all places and expanded it.
        Folded tmel_work in tmel.
        Added more kernel doc as relevant.
        Removed __packed in all places, as not required.
        Renamed all functions to have tmel_ prefixes.
        Used readl/writel in all places.
	Added Inline for all required functions.
        Removed redundant type conversions.
        Removed redundant 'goto's
        Added __free macro
        Fixed Linux std errno in tmel_sec_boot_auth/teardown
        Added spinlock in qmp_startup
        Used of_mbox_index_xlate and dropped the tmel_qmp_mbox_xlate
        Updated header file to have only mbox consumer required and moved rest to .c file
        Fixed the TMEL_MSG macros to use standard GENMASK
        Moved the irq registration to end of probe

    Following tests were done and no issues.

       *)  Checkpatch
       *)  Codespell
       *)  Sparse
       *)  kerneldoc check
       *)  Kernel lock debugging
       *)  dt_binding_check and dtbs_check

[V3]

        Fixed wrong code/comments wrappings.
        Fixed Kconfig and Makefile entries to right place.
        Removed unused headers inclusion.
        Fixed locking, removed the mutexes and having only tx spinlock.
        Removed the use of global ptr for tmel, made it as device specific.
        Replaced pr_err/pr_debug with dev_err/dev_dbg in all places.
        Fixed usage of dev_err_probe.
        Fixed xlate callback as per comments.
        Used devm equivalents and kcalloc version as per comments.
        Removed all un-nessecary wrapper macros for register access, inlined it
        as per comments.
        Re-organised the function layout as per comments and make it more readable.
        Removed the pictures in headers files as per comments.
        Used Field_prep/get as per comments.
        Fixed Kernel test reported issues.
        Fixed all other comments as well.

    Following tests were done and no issues.

       *)  Checkpatch
       *)  Codespell
       *)  Sparse
       *)  kerneldoc check
       *)  Kernel lock debugging
       *)  dt_binding_check and dtbs_check

[v2]
     Added HW description in the bindings patch.
     Fixed review comments for bindings from Krzysztof and Dmitry
     Changed patch#2 driver to add work for mailbox tx processing
     Cleaned up patch#2 for some checkpatch warnings.
     There are some checkpatch [CHECK] like below, which looks like false positive.

	CHECK: Macro argument 'm' may be better as '(m)' to avoid precedence issues
	#1072: FILE: include/linux/mailbox/tmelcom-qmp.h:40:
	+#define TMEL_MSG_UID_CREATE(m, a)      ((u32)(((m & 0xff) << 8) | (a & 0xff)))

[v1]
      RFC Post

Sricharan Ramabadhran (2):
  dt-bindings: mailbox: Document qcom,ipq5424-tmel
  mailbox: tmelite-qmp: Introduce TMEL QMP mailbox driver

 .../bindings/mailbox/qcom,ipq5424-tmel.yaml   |  60 ++
 drivers/mailbox/Kconfig                       |  10 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/qcom-tmel-qmp.c               | 947 ++++++++++++++++++
 include/linux/mailbox/tmelcom-qmp.h           |  65 ++
 5 files changed, 1084 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/qcom,ipq5424-tmel.yaml
 create mode 100644 drivers/mailbox/qcom-tmel-qmp.c
 create mode 100644 include/linux/mailbox/tmelcom-qmp.h

-- 
2.34.1

[PATCH V4 1/2] dt-bindings: mailbox: Document qcom,ipq5424-tmel

[Sricharan R]

From: Sricharan Ramabadhran <quic_srichara@quicinc.com>

TMEL(Trust Management Engine Lite) subsystem provides different kinds of
services like secureboot, remote image authentication, key management,
crypto, OEM provisioning etc.

The QMP mailbox is the primary means of communication between TMEL SS and
other subsystem on the SoC. A dedicated pair of inbound and outbound
mailboxes is implemented for each subsystem/external execution environment
which needs to communicate with TMEL for security services. The inbound
mailboxes are used to send IPC requests to TMEL, which are then processed
by TMEL firmware and accordingly the responses are sent back via outbound
mailboxes.

It is an IPC transport protocol which is light weight and supports a subset
of API's. It handles link initialization, negotiation, establishment and
communication across client(CPU/BTSS/AUDIOSS) and server(TMEL SS).

   -----------------------------------------------       ---------------------------------------------------
  |                                              |       |                                                 |
  |                 SOC  CLIENT                  | SOC   |                TMEL  SS                         |
  |                                              | AHB   |                                                 |
  |     ----------    ---------   ---------      |       | ------    -------     --------    ------------  |
  |     |        |    |       |   |       |      | WO    | |     | R |     |     |      |    |SERVICES   | |
  |     | CPU    |<-->| TMEL  |<->|       |------------->| | IN  |-->|     |     | TMEL |    |--------   | |
  |     |        |    | COM   |   | QMP   |      | RO    | |     | W | QMP |<--->| COM  |<-->| a) ATTEST | |
  |     |        |    |       |   |       |<-------------| | OUT |<--|     |     |      |    | b) CRYPTO | |
  |     |        |    |       |   |       |      |       | |     |   |     |     |      |    | .. more   | |
  |     ---------     ---------   ---------      |       | ------    -------     -------     ------------  |
  |                                              |       |                                                 |
   -----------------------------------------------       --------------------------------------------------

This binding describes the component responsible for communication between
the TMEL subsystem and the TMEL client (CPU/BTSS/AUDIOSS),
used for security services like secure image authentication, enable/disable
efuses, crypto services. Each client in the SoC has its own block of message
RAM and IRQ for communication with the TMEL SS.

Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
---
[v4]
	Fixed TMEL naming and added expansion
        Fixed wrappings for 80 columns

[V3]
        Fixed wrappings.
        Made mailbox-cells as a required property and changed value to '1'.
        Fixed to use compatible as filename.
        Renamed compatible as per Krzystof's comments.
        Dropped unused label.

[V2]
        Added HW description as per comments.
        Removed the fallback compatible.
        Fixed naming convention to TME-L in all places.
        Fixed indendation for example.
        Removed the 'description' for some items.

[V1]
	RFC Post.

 .../bindings/mailbox/qcom,ipq5424-tmel.yaml   | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/qcom,ipq5424-tmel.yaml

diff --git a/Documentation/devicetree/bindings/mailbox/qcom,ipq5424-tmel.yaml b/Documentation/devicetree/bindings/mailbox/qcom,ipq5424-tmel.yaml
new file mode 100644
index 000000000000..5bdeab166a1f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/qcom,ipq5424-tmel.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/qcom,ipq5424-tmel.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm TMEL IPCC channel
+
+maintainers:
+  - Sricharan Ramabadhran <quic_srichara@quicinc.com>
+
+description:
+  TMEL(Trust Management Engine Lite) SS provides different kinds of services
+  like secureboot, remote image authentication, key management, crypto, OEM
+  provisioning etc.
+
+  The QMP mailbox is the primary means of communication between TMEL SS and
+  other subsystem on the SoC. A dedicated pair of inbound and outbound mailbox
+  is implemented for each subsystem/external execution environment which needs
+  to communicate with TMEL for security services. The inbound mailboxes are used
+  to send IPC requests to TMEL, which are then processed by TMEL firmware and
+  accordingly the responses are sent back via outbound mailboxes.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - qcom,ipq5424-tmel
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  mboxes:
+    maxItems: 1
+
+  "#mbox-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - mboxes
+  - "#mbox-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    mailbox@32090000 {
+        compatible = "qcom,ipq5424-tmel";
+        reg = <0x32090000 0x2000>;
+        interrupts = <GIC_SPI 126 IRQ_TYPE_EDGE_RISING>;
+        mboxes = <&apcs_glb 20>;
+        #mbox-cells = <1>;
+    };
-- 
2.34.1

[PATCH V4 2/2] mailbox: tmelite-qmp: Introduce TMEL QMP mailbox driver

[Sricharan R]

From: Sricharan Ramabadhran <quic_srichara@quicinc.com>

This mailbox facilitates the communication between the TMEL server
subsystem (Trust Management Engine Lite) and the TMEL client
(APPSS/BTSS/AUDIOSS), used for secure services like secure image
authentication, enable/disable efuses, crypto services etc. Each client in
the SoC has its own block of message RAM and IRQ for communication with the
TMEL SS. The protocol used to communicate in the message RAM is known as
Qualcomm Messaging Protocol (QMP).

Remote proc driver subscribes to this mailbox and uses the
mbox_send_message to use TMEL to securely authenticate/teardown the images.

Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
---
[V4]
        Fixed TME-L naming in all places and expanded it.
        Folded tmel_work in tmel.
        Added more kernel doc as relevant.
        Removed __packed in all places, as not required.
        Renamed all functions to have tmel_ prefixes.
        Used readl/writel in all places.
	Added Inline for all required functions.
        Removed redundant type conversions.
        Removed redundant 'goto's
        Added __free macro
        Fixed Linux std errno in tmel_sec_boot_auth/teardown
        Added spinlock in qmp_startup
        Used of_mbox_index_xlate and dropped the tmel_qmp_mbox_xlate
        Updated header file to have only mbox consumer required and moved rest to .c file
        Fixed the TMEL_MSG macros to use standard GENMASK
        Moved the irq registration to end of probe

    Following tests were done and no issues.

       *)  Checkpatch
       *)  Codespell
       *)  Sparse
       *)  kerneldoc check
       *)  Kernel lock debugging
       *)  dt_binding_check and dtbs_check

[V3]

        Fixed wrong code/comments wrappings.
        Fixed Kconfig and Makefile entries to right place.
        Removed unused headers inclusion.
        Fixed locking, removed the mutexes and having only tx spinlock.
        Removed the use of global ptr for tmel, made it as device specific.
        Replaced pr_err/pr_debug with dev_err/dev_dbg in all places.
        Fixed usage of dev_err_probe.
        Fixed xlate callback as per comments.
        Used devm equivalents and kcalloc version as per comments.
        Removed all un-nessecary wrapper macros for register access, inlined it
        as per comments.
        Re-organised the function layout as per comments and make it more readable.
        Removed the pictures in headers files as per comments.
        Used Field_prep/get as per comments.
        Fixed Kernel test reported issues.
        Fixed all other comments as well.

    Following tests were done and no issues.

       *)  Checkpatch
       *)  Codespell
       *)  Sparse
       *)  kerneldoc check
       *)  Kernel lock debugging
       *)  dt_binding_check and dtbs_check

[v2]
     Added HW description in the bindings patch.
     Fixed review comments for bindings from Krzysztof and Dmitry
     Changed patch#2 driver to add work for mailbox tx processing
     Cleaned up patch#2 for some checkpatch warnings.
     There are some checkpatch [CHECK] like below, which looks like false positive.

	CHECK: Macro argument 'm' may be better as '(m)' to avoid precedence issues
	#1072: FILE: include/linux/mailbox/tmelcom-qmp.h:40:
	+#define TMEL_MSG_UID_CREATE(m, a)      ((u32)(((m & 0xff) << 8) | (a & 0xff)))

[v1]
      RFC Post

 drivers/mailbox/Kconfig             |  10 +
 drivers/mailbox/Makefile            |   2 +
 drivers/mailbox/qcom-tmel-qmp.c     | 947 ++++++++++++++++++++++++++++
 include/linux/mailbox/tmelcom-qmp.h |  65 ++
 4 files changed, 1024 insertions(+)
 create mode 100644 drivers/mailbox/qcom-tmel-qmp.c
 create mode 100644 include/linux/mailbox/tmelcom-qmp.h

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index ed52db272f4d..a574c421227a 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -320,6 +320,16 @@ config QCOM_IPCC
 	  acts as an interrupt controller for receiving interrupts from clients.
 	  Say Y here if you want to build this driver.
 
+config QCOM_TMEL_QMP_MAILBOX
+	tristate "QCOM Mailbox Protocol(QMP) for TMEL SS"
+	depends on ARCH_QCOM || COMPILE_TEST
+	help
+	  Say yes to add support for the QMP Mailbox Protocol driver for
+	  Trust Management Engine Lite Sub System (TMEL SS).
+	  QMP is a lightweight communication protocol for sending messages to
+	  TMEL. This protocol fits into the Generic Mailbox Framework.
+	  QMP uses a mailbox registers.
+
 config THEAD_TH1520_MBOX
 	tristate "T-head TH1520 Mailbox"
 	depends on ARCH_THEAD || COMPILE_TEST
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 9a1542b55539..31d5a4f96690 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -69,4 +69,6 @@ obj-$(CONFIG_QCOM_CPUCP_MBOX)	+= qcom-cpucp-mbox.o
 
 obj-$(CONFIG_QCOM_IPCC)		+= qcom-ipcc.o
 
+obj-$(CONFIG_QCOM_TMEL_QMP_MAILBOX) += qcom-tmel-qmp.o
+
 obj-$(CONFIG_THEAD_TH1520_MBOX)	+= mailbox-th1520.o
diff --git a/drivers/mailbox/qcom-tmel-qmp.c b/drivers/mailbox/qcom-tmel-qmp.c
new file mode 100644
index 000000000000..b8b8573f4d51
--- /dev/null
+++ b/drivers/mailbox/qcom-tmel-qmp.c
@@ -0,0 +1,947 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/cleanup.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/mailbox_client.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox/tmelcom-qmp.h>
+#include <linux/platform_device.h>
+#include <linux/uio.h>
+
+#define QMP_NUM_CHANS		0x1
+#define QMP_TOUT_MS		1000
+#define QMP_CTRL_DATA_SIZE	4
+#define QMP_MAX_PKT_SIZE	0x18
+#define QMP_UCORE_DESC_OFFSET	0x1000
+#define QMP_SEND_TIMEOUT	30000
+
+#define QMP_HW_MBOX_SIZE		32
+#define QMP_MBOX_RSV_SIZE		4
+#define QMP_MBOX_IPC_PACKET_SIZE	(QMP_HW_MBOX_SIZE - QMP_CTRL_DATA_SIZE - QMP_MBOX_RSV_SIZE)
+#define QMP_MBOX_IPC_MAX_PARAMS		5
+
+#define QMP_MAX_PARAM_IN_PARAM_ID	14
+#define QMP_PARAM_CNT_FOR_OUTBUF	3
+#define QMP_SRAM_IPC_MAX_PARAMS		(QMP_MAX_PARAM_IN_PARAM_ID * QMP_PARAM_CNT_FOR_OUTBUF)
+#define QMP_SRAM_IPC_MAX_BUF_SIZE	(QMP_SRAM_IPC_MAX_PARAMS * sizeof(u32))
+
+#define TMEL_ERROR_GENERIC		(0x1u)
+#define TMEL_ERROR_NOT_SUPPORTED	(0x2u)
+#define TMEL_ERROR_BAD_PARAMETER	(0x3u)
+#define TMEL_ERROR_BAD_MESSAGE		(0x4u)
+#define TMEL_ERROR_BAD_ADDRESS		(0x5u)
+#define TMEL_ERROR_TMELCOM_FAILURE	(0x6u)
+#define TMEL_ERROR_TMEL_BUSY		(0x7u)
+
+/*
+ * mbox data can be shared over mem or sram
+ */
+enum ipc_type {
+	IPC_MBOX_MEM,
+	IPC_MBOX_SRAM,
+};
+
+/*
+ * mbox header indicates the type of payload and action required.
+ */
+struct ipc_header {
+	u8 ipc_type:1;
+	u8 msg_len:7;
+	u8 msg_type;
+	u8 action_id;
+	s8 response;
+};
+
+struct mbox_payload {
+	u32 param[QMP_MBOX_IPC_MAX_PARAMS];
+};
+
+struct sram_payload {
+	u32 payload_ptr;
+	u32 payload_len;
+};
+
+union ipc_payload {
+	struct mbox_payload mbox_payload;
+	struct sram_payload sram_payload;
+};
+
+struct tmel_ipc_pkt {
+	struct ipc_header msg_hdr;
+	union ipc_payload payload;
+};
+
+/**
+ * enum qmp_local_state - definition of the local state machine
+ * @LINK_DISCONNECTED: Init state, waiting for ucore to start
+ * @LINK_NEGOTIATION: Set local link state to up, wait for ucore ack
+ * @LINK_CONNECTED: Link state up, channel not connected
+ * @LOCAL_CONNECTING: Channel opening locally, wait for ucore ack
+ * @CHANNEL_CONNECTED: Channel fully opened
+ * @LOCAL_DISCONNECTING: Channel disconnected locally, wait for ucore ack
+ */
+enum qmp_local_state {
+	LINK_DISCONNECTED,
+	LINK_NEGOTIATION,
+	LINK_CONNECTED,
+	LOCAL_CONNECTING,
+	CHANNEL_CONNECTED,
+	LOCAL_DISCONNECTING,
+};
+
+/**
+ * struct qmp_channel_desc - IPC bits
+ * @bits: Var to access each member
+ * @val: u32 representation of above
+ */
+union qmp_channel_desc {
+	struct {
+		u32 link_state:1;
+		u32 link_state_ack:1;
+		u32 ch_state:1;
+		u32 ch_state_ack:1;
+		u32 tx:1;
+		u32 tx_ack:1;
+		u32 rx_done:1;
+		u32 rx_done_ack:1;
+		u32 reserved:8;
+		u32 frag_size:8;
+		u32 rem_frag_count:8;
+	} bits;
+	unsigned int val;
+};
+
+/**
+ * struct qmp_device - local information for managing a single mailbox
+ * @dev: The device that corresponds to this mailbox
+ * @mcore_desc: Local core (APSS) mailbox descriptor
+ * @ucore_desc: Remote core (TME-L) mailbox descriptor
+ * @mcore: Local core (APSS) channel descriptor
+ * @ucore: Remote core (TME-L) channel descriptor
+ * @rx_pkt: Buffer to pass to client, holds received data from mailbox
+ * @mbox_client: Mailbox client for the IPC interrupt
+ * @mbox_chan: Mailbox client chan for the IPC interrupt
+ * @local_state: Current state of mailbox protocol
+ * @tx_lock: Serialize access for writes to mailbox
+ * @link_complete: Use to block until link negotiation with remote proc
+ * @ch_complete: Use to block until the channel is fully opened
+ * @tx_sent: True if tx is sent and remote proc has not sent ack
+ */
+struct qmp_device {
+	struct device *dev;
+
+	void __iomem *mcore_desc;
+	void __iomem *ucore_desc;
+	union qmp_channel_desc mcore;
+	union qmp_channel_desc ucore;
+
+	struct kvec rx_pkt;
+
+	struct mbox_client mbox_client;
+	struct mbox_chan *mbox_chan;
+
+	enum qmp_local_state local_state;
+
+	/*
+	 * Serialize access to mcore IPC descriptors.
+	 * mcore refers to the IPC request descriptors sent to TMEL,
+	 * protecting it from various SM transitions using this.
+	 */
+	spinlock_t tx_lock;
+
+	struct completion link_complete;
+	struct completion ch_complete;
+
+	atomic_t tx_sent;
+};
+
+/**
+ * struct tmel - tmel controller instance
+ * @dev: The device that corresponds to this mailbox
+ * @ctrl: Mailbox controller for use by tmel clients
+ * @mdev: qmp_device associated with this tmel instance
+ * @pkt: Buffer from client, to be sent over mailbox
+ * @ipc_pkt: wrapper used for prepare/un_prepare
+ * @sram_dma_addr: mailbox sram address to copy the data
+ * @waitq: Use to wait for posted messages completion
+ * @rx_done: Use to indicate receive completion from remote
+ * @twork: worker for posting the client req to tmel ctrl
+ * @data: client data to be sent for the current request
+ */
+struct tmel {
+	struct device *dev;
+	struct mbox_controller ctrl;
+	struct qmp_device *mdev;
+	struct kvec pkt;
+	struct tmel_ipc_pkt *ipc_pkt;
+	dma_addr_t sram_dma_addr;
+	wait_queue_head_t waitq;
+	bool rx_done;
+	struct work_struct twork;
+	void *data;
+};
+
+struct tmel_msg_param_type_buf_in {
+	u32 buf;
+	u32 buf_len;
+};
+
+struct tmel_secboot_sec_auth_req {
+	u32 sw_id;
+	struct tmel_msg_param_type_buf_in elf_buf;
+	struct tmel_msg_param_type_buf_in region_list;
+	u32 relocate;
+};
+
+struct tmel_secboot_sec_auth_resp {
+	u32 first_seg_addr;
+	u32 first_seg_len;
+	u32 entry_addr;
+	u32 extended_error;
+	u32 status;
+};
+
+struct tmel_secboot_sec_auth {
+	struct tmel_secboot_sec_auth_req req;
+	struct tmel_secboot_sec_auth_resp resp;
+};
+
+struct tmel_secboot_sec {
+	struct device *dev;
+	void *elf_buf;
+	struct tmel_secboot_sec_auth msg;
+};
+
+struct tmel_secboot_teardown_req {
+	u32 sw_id;
+	u32 secondary_sw_id;
+};
+
+struct tmel_secboot_teardown_resp {
+	u32 status;
+};
+
+struct tmel_secboot_teardown {
+	struct tmel_secboot_teardown_req req;
+	struct tmel_secboot_teardown_resp resp;
+};
+
+/**
+ * tmel_qmp_send_irq() - send an irq to a remote entity as an event signal.
+ * @mdev: Which remote entity that should receive the irq.
+ */
+static inline void tmel_qmp_send_irq(struct qmp_device *mdev)
+{
+	writel(mdev->mcore.val, mdev->mcore_desc);
+	/* Ensure desc update is visible before IPC */
+	wmb();
+
+	dev_dbg(mdev->dev, "%s: mcore 0x%x ucore 0x%x", __func__,
+		mdev->mcore.val, mdev->ucore.val);
+
+	mbox_send_message(mdev->mbox_chan, NULL);
+	mbox_client_txdone(mdev->mbox_chan, 0);
+}
+
+/**
+ * tmel_qmp_send_data() - Send the data to remote and notify.
+ * @mdev: qmp_device to send the data to.
+ * @data: Data to be sent to remote processor, should be in the format of
+ *	  a kvec.
+ *
+ * Copy the data to the channel's mailbox and notify remote subsystem of new
+ * data. This function will return an error if the previous message sent has
+ * not been read.
+ */
+static inline int tmel_qmp_send_data(struct qmp_device *mdev, void *data)
+{
+	struct kvec *pkt = (struct kvec *)data;
+	void __iomem *addr;
+	unsigned long flags;
+
+	if (pkt->iov_len > QMP_MAX_PKT_SIZE) {
+		dev_err(mdev->dev, "Unsupported packet size");
+		return -EINVAL;
+	}
+
+	if (atomic_read(&mdev->tx_sent))
+		return -EAGAIN;
+
+	dev_dbg(mdev->dev, "%s: mcore 0x%x ucore 0x%x", __func__,
+		mdev->mcore.val, mdev->ucore.val);
+
+	addr = mdev->mcore_desc + QMP_CTRL_DATA_SIZE;
+	memcpy_toio(addr, pkt->iov_base, pkt->iov_len);
+
+	mdev->mcore.bits.frag_size = pkt->iov_len;
+	mdev->mcore.bits.rem_frag_count = 0;
+
+	dev_dbg(mdev->dev, "Copied buffer to mbox, sz: %d",
+		mdev->mcore.bits.frag_size);
+
+	atomic_set(&mdev->tx_sent, 1);
+
+	spin_lock_irqsave(&mdev->tx_lock, flags);
+	mdev->mcore.bits.tx = !(mdev->mcore.bits.tx);
+	tmel_qmp_send_irq(mdev);
+	spin_unlock_irqrestore(&mdev->tx_lock, flags);
+
+	return 0;
+}
+
+/**
+ * tmel_qmp_notify_client() - Notify the tmel client about remote data.
+ * @tdev: tmel device to notify.
+ * @message: response pkt from remote processor, should be in format of kvec.
+ *
+ * Wakeup the clients after receiving data from the remote.
+ */
+static inline void tmel_qmp_notify_client(struct tmel *tdev, void *message)
+{
+	struct kvec *pkt = NULL;
+
+	if (!message) {
+		dev_err(tdev->dev, "spurious message received\n");
+		goto notify_fail;
+	}
+
+	if (tdev->rx_done) {
+		dev_err(tdev->dev, "tmel response pending\n");
+		goto notify_fail;
+	}
+
+	pkt = (struct kvec *)message;
+	tdev->pkt.iov_len = pkt->iov_len;
+	tdev->pkt.iov_base = pkt->iov_base;
+	tdev->rx_done = true;
+
+notify_fail:
+	wake_up_interruptible(&tdev->waitq);
+}
+
+/**
+ * tmel_qmp_recv_data() - Receive data and send ack.
+ * @tdev: tmel device that received the notification.
+ * @mbox_of: offset of mailbox after QMP Control data.
+ *
+ * Copies data from mailbox and passes to the client upon receiving data
+ * available notification. Also acknowledges the read completion.
+ */
+static inline void tmel_qmp_recv_data(struct tmel *tdev, u32 mbox_of)
+{
+	struct qmp_device *mdev = tdev->mdev;
+	void __iomem *addr;
+	struct kvec *pkt;
+
+	addr = mdev->ucore_desc + mbox_of;
+	pkt = &mdev->rx_pkt;
+	pkt->iov_len = mdev->ucore.bits.frag_size;
+
+	memcpy_fromio(pkt->iov_base, addr, pkt->iov_len);
+	mdev->mcore.bits.tx_ack = mdev->ucore.bits.tx;
+	dev_dbg(mdev->dev, "%s: Send RX data to TMEL Client", __func__);
+	tmel_qmp_notify_client(tdev, pkt);
+
+	mdev->mcore.bits.rx_done = !(mdev->mcore.bits.rx_done);
+	tmel_qmp_send_irq(mdev);
+}
+
+/**
+ * tmel_qmp_clr_mcore_ch_state() - Clear the mcore state of a mailbox.
+ * @mdev: mailbox device to be initialized.
+ */
+static inline void tmel_qmp_clr_mcore_ch_state(struct qmp_device *mdev)
+{
+	/* Clear all fields except link_state */
+	mdev->mcore.bits.ch_state = 0;
+	mdev->mcore.bits.ch_state_ack = 0;
+	mdev->mcore.bits.tx =  0;
+	mdev->mcore.bits.tx_ack =  0;
+	mdev->mcore.bits.rx_done = 0;
+	mdev->mcore.bits.rx_done_ack = 0;
+	mdev->mcore.bits.frag_size = 0;
+	mdev->mcore.bits.rem_frag_count = 0;
+}
+
+/**
+ * tmel_qmp_rx() - Handle incoming messages from remote processor.
+ * @tdev: tmel device to send the event to.
+ */
+static inline void tmel_qmp_rx(struct tmel *tdev)
+{
+	struct qmp_device *mdev = tdev->mdev;
+	unsigned long flags;
+
+	/* read remote_desc from mailbox register */
+	mdev->ucore.val = readl(mdev->ucore_desc);
+
+	dev_dbg(mdev->dev, "%s: mcore 0x%x ucore 0x%x", __func__,
+		mdev->mcore.val, mdev->ucore.val);
+
+	spin_lock_irqsave(&mdev->tx_lock, flags);
+
+	/* Check if remote link down */
+	if (mdev->local_state >= LINK_CONNECTED &&
+	    !(mdev->ucore.bits.link_state)) {
+		mdev->local_state = LINK_NEGOTIATION;
+		mdev->mcore.bits.link_state_ack = mdev->ucore.bits.link_state;
+		tmel_qmp_send_irq(mdev);
+		spin_unlock_irqrestore(&mdev->tx_lock, flags);
+		return;
+	}
+
+	switch (mdev->local_state) {
+	case LINK_NEGOTIATION:
+		if (!(mdev->mcore.bits.link_state) ||
+		    !(mdev->ucore.bits.link_state)) {
+			dev_err(mdev->dev, "rx irq:link down state\n");
+			break;
+		}
+		tmel_qmp_clr_mcore_ch_state(mdev);
+		mdev->mcore.bits.link_state_ack = mdev->ucore.bits.link_state;
+		mdev->local_state = LINK_CONNECTED;
+		complete_all(&mdev->link_complete);
+		dev_dbg(mdev->dev, "Set to link connected");
+		break;
+	case LINK_CONNECTED:
+		/* No need to handle until local opens */
+		break;
+	case LOCAL_CONNECTING:
+		/* Ack to remote ch_state change */
+		mdev->mcore.bits.ch_state_ack = mdev->ucore.bits.ch_state;
+		mdev->local_state = CHANNEL_CONNECTED;
+		complete_all(&mdev->ch_complete);
+		dev_dbg(mdev->dev, "Set to channel connected");
+		tmel_qmp_send_irq(mdev);
+		break;
+	case CHANNEL_CONNECTED:
+		/* Check for remote channel down */
+		if (!(mdev->ucore.bits.ch_state)) {
+			mdev->local_state = LOCAL_CONNECTING;
+			mdev->mcore.bits.ch_state_ack = mdev->ucore.bits.ch_state;
+			dev_dbg(mdev->dev, "Remote Disconnect");
+			tmel_qmp_send_irq(mdev);
+		}
+
+		/* Check TX done */
+		if (atomic_read(&mdev->tx_sent) &&
+		    mdev->ucore.bits.rx_done != mdev->mcore.bits.rx_done_ack) {
+			/* Ack to remote */
+			mdev->mcore.bits.rx_done_ack = mdev->ucore.bits.rx_done;
+			atomic_set(&mdev->tx_sent, 0);
+			dev_dbg(mdev->dev, "TX flag cleared");
+		}
+
+		/* Check if remote is Transmitting */
+		if (!(mdev->ucore.bits.tx != mdev->mcore.bits.tx_ack))
+			break;
+		if (mdev->ucore.bits.frag_size == 0 ||
+		    mdev->ucore.bits.frag_size > QMP_MAX_PKT_SIZE) {
+			dev_err(mdev->dev, "Rx frag size error %d\n",
+				mdev->ucore.bits.frag_size);
+			break;
+		}
+		tmel_qmp_recv_data(tdev, QMP_CTRL_DATA_SIZE);
+		break;
+	case LOCAL_DISCONNECTING:
+		if (!(mdev->mcore.bits.ch_state)) {
+			tmel_qmp_clr_mcore_ch_state(mdev);
+			mdev->local_state = LINK_CONNECTED;
+			dev_dbg(mdev->dev, "Channel closed");
+			reinit_completion(&mdev->ch_complete);
+		}
+
+		break;
+	default:
+		dev_err(mdev->dev, "Local Channel State corrupted\n");
+	}
+	spin_unlock_irqrestore(&mdev->tx_lock, flags);
+}
+
+/**
+ * tmel_qmp_irq_handler() - Handle incoming messages from remote processor.
+ * @irq: ipc interrupt from remote
+ * @priv: ptr to the corresponding tmel device.
+ */
+static inline irqreturn_t tmel_qmp_irq_handler(int irq, void *priv)
+{
+	struct tmel *tdev = (struct tmel *)priv;
+
+	tmel_qmp_rx(tdev);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * tmel_prepare_msg() - copies the payload to the mbox destination
+ * @tdev: the tmel device
+ * @msg_uid: msg_type/action_id combo
+ * @msg_buf: payload to be sent
+ * @msg_size: size of the payload
+ */
+static inline int tmel_prepare_msg(struct tmel *tdev, u32 msg_uid,
+				   void *msg_buf, size_t msg_size)
+{
+	struct tmel_ipc_pkt *ipc_pkt = tdev->ipc_pkt;
+	struct ipc_header *msg_hdr = &ipc_pkt->msg_hdr;
+	struct mbox_payload *mbox_payload = &ipc_pkt->payload.mbox_payload;
+	struct sram_payload *sram_payload = &ipc_pkt->payload.sram_payload;
+	int ret;
+
+	memset(ipc_pkt, 0, sizeof(struct tmel_ipc_pkt));
+
+	msg_hdr->msg_type = TMEL_MSG_UID_MSG_TYPE(msg_uid);
+	msg_hdr->action_id = TMEL_MSG_UID_ACTION_ID(msg_uid);
+
+	dev_dbg(tdev->dev, "uid: %d, msg_size: %zu msg_type:%d, action_id:%d\n",
+		msg_uid, msg_size, msg_hdr->msg_type, msg_hdr->action_id);
+
+	if (sizeof(struct ipc_header) + msg_size <= QMP_MBOX_IPC_PACKET_SIZE) {
+		/* Mbox only */
+		msg_hdr->ipc_type = IPC_MBOX_MEM;
+		msg_hdr->msg_len = msg_size;
+		memcpy((void *)mbox_payload, msg_buf, msg_size);
+	} else if (msg_size <= QMP_SRAM_IPC_MAX_BUF_SIZE) {
+		/* SRAM */
+		msg_hdr->ipc_type = IPC_MBOX_SRAM;
+		msg_hdr->msg_len = 8;
+
+		tdev->sram_dma_addr = dma_map_single(tdev->dev, msg_buf,
+						     msg_size,
+						     DMA_BIDIRECTIONAL);
+		ret = dma_mapping_error(tdev->dev, tdev->sram_dma_addr);
+		if (ret) {
+			dev_err(tdev->dev, "SRAM DMA mapping error: %d\n", ret);
+			return ret;
+		}
+
+		sram_payload->payload_ptr = tdev->sram_dma_addr;
+		sram_payload->payload_len = msg_size;
+	} else {
+		dev_err(tdev->dev, "Invalid payload length: %zu\n", msg_size);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/**
+ * tmel_unprepare_message() - Get the response data back for client
+ * @tdev: the tmel device
+ * @msg_buf: payload to be sent
+ * @msg_size: size of the payload
+ */
+static inline void tmel_unprepare_message(struct tmel *tdev, void *msg_buf, size_t msg_size)
+{
+	struct tmel_ipc_pkt *ipc_pkt = (struct tmel_ipc_pkt *)tdev->pkt.iov_base;
+	struct mbox_payload *mbox_payload = &ipc_pkt->payload.mbox_payload;
+
+	if (ipc_pkt->msg_hdr.ipc_type == IPC_MBOX_MEM) {
+		memcpy(msg_buf, mbox_payload, msg_size);
+	} else if (ipc_pkt->msg_hdr.ipc_type == IPC_MBOX_SRAM) {
+		dma_unmap_single(tdev->dev, tdev->sram_dma_addr, msg_size, DMA_BIDIRECTIONAL);
+		tdev->sram_dma_addr = 0;
+	}
+}
+
+static inline bool tmel_rx_done(struct tmel *tdev)
+{
+	return tdev->rx_done;
+}
+
+/**
+ * tmel_process_request() - process client msg and wait for response
+ * @tdev: the tmel device
+ * @msg_uid: msg_type/action_id combo
+ * @msg_buf: payload to be sent
+ * @msg_size: size of the payload
+ */
+static inline int tmel_process_request(struct tmel *tdev, u32 msg_uid,
+				       void *msg_buf, size_t msg_size)
+{
+	struct qmp_device *mdev = tdev->mdev;
+	struct tmel_ipc_pkt *resp_ipc_pkt;
+	struct mbox_chan *chan;
+	unsigned long jiffies;
+	long time_left = 0;
+	int ret = 0;
+
+	chan = &tdev->ctrl.chans[0];
+
+	if (!msg_buf || !msg_size) {
+		dev_err(tdev->dev, "Invalid msg_buf or msg_size\n");
+		return -EINVAL;
+	}
+
+	tdev->rx_done = false;
+
+	ret = tmel_prepare_msg(tdev, msg_uid, msg_buf, msg_size);
+	if (ret)
+		return ret;
+
+	tdev->pkt.iov_len = sizeof(struct tmel_ipc_pkt);
+	tdev->pkt.iov_base = (void *)tdev->ipc_pkt;
+
+	tmel_qmp_send_data(mdev, &tdev->pkt);
+	jiffies = msecs_to_jiffies(QMP_SEND_TIMEOUT);
+
+	time_left = wait_event_interruptible_timeout(tdev->waitq,
+						     tmel_rx_done(tdev),
+						     jiffies);
+
+	if (!time_left) {
+		dev_err(tdev->dev, "Request timed out\n");
+		atomic_set(&mdev->tx_sent, 0);
+		mbox_chan_txdone(chan, ret);
+		return -ETIMEDOUT;
+	}
+
+	if (tdev->pkt.iov_len != sizeof(struct tmel_ipc_pkt))
+		return -EPROTO;
+
+	resp_ipc_pkt = (struct tmel_ipc_pkt *)tdev->pkt.iov_base;
+	tmel_unprepare_message(tdev, msg_buf, msg_size);
+	tdev->rx_done = false;
+
+	return resp_ipc_pkt->msg_hdr.response;
+}
+
+/**
+ * tmel_secboot_sec_free() - Release the dma alloc and kmalloc'ed memory
+ * @ptr: Address of the tmel_secboot_sec wrapper for dma and kmalloc region.
+ */
+void tmel_secboot_sec_free(void *ptr)
+{
+	struct tmel_secboot_sec *smsg = ptr;
+	void *elf_buf = smsg->elf_buf;
+	dma_addr_t elf_buf_phys;
+	u32 size;
+
+	elf_buf_phys = smsg->msg.req.elf_buf.buf;
+	size = smsg->msg.req.elf_buf.buf_len;
+	dma_free_coherent(smsg->dev, size, elf_buf, elf_buf_phys);
+	kfree(smsg);
+}
+
+/**
+ * tmel_secboot_sec_auth() - authenticate the remote subsys image
+ * @tdev: the tmel device
+ * @sw_id: pas_id of the remote
+ * @metadata: payload to be sent
+ * @size: size of the payload
+ */
+static inline int tmel_secboot_sec_auth(struct tmel *tdev, u32 sw_id,
+					void *metadata, size_t size)
+{
+	struct tmel_secboot_sec *smsg __free(tmel_secboot_sec_f) = NULL;
+	struct device *dev = tdev->dev;
+	dma_addr_t elf_buf_phys;
+	void *elf_buf;
+	int ret;
+
+	if (!dev || !metadata)
+		return -EINVAL;
+
+	smsg = kzalloc(sizeof(*smsg), GFP_KERNEL);
+
+	elf_buf = dma_alloc_coherent(dev, size, &elf_buf_phys, GFP_KERNEL);
+	if (!elf_buf)
+		return -ENOMEM;
+
+	memcpy(elf_buf, metadata, size);
+
+	smsg->dev = dev;
+	smsg->elf_buf = elf_buf;
+
+	smsg->msg.req.sw_id = sw_id;
+	smsg->msg.req.elf_buf.buf = (u32)elf_buf_phys;
+	smsg->msg.req.elf_buf.buf_len = (u32)size;
+
+	ret = tmel_process_request(tdev, TMEL_MSG_UID_SECBOOT_SEC_AUTH,
+				   &smsg->msg,
+				   sizeof(struct tmel_secboot_sec_auth));
+	if (ret) {
+		dev_err(dev, "Failed to send IPC: %d\n", ret);
+	} else if (smsg->msg.resp.status) {
+		dev_err(dev, "Failed with status: %d", smsg->msg.resp.status);
+		ret = smsg->msg.resp.status ? -EINVAL : 0;
+	} else if (smsg->msg.resp.extended_error) {
+		dev_err(dev, "Failed with error: %d", smsg->msg.resp.extended_error);
+		ret = smsg->msg.resp.extended_error ? -EINVAL : 0;
+	}
+
+	return ret;
+}
+
+/**
+ * tmel_secboot_teardown() - teardown the remote subsys
+ * @tdev: tmel device
+ * @sw_id: pas_id of the remote
+ * @secondary_sw_id: extra argument for the pas_id
+ */
+static inline int tmel_secboot_teardown(struct tmel *tdev, u32 sw_id,
+					u32 secondary_sw_id)
+{
+	struct tmel_secboot_teardown msg = {0};
+	struct device *dev = tdev->dev;
+	int ret;
+
+	if (!dev)
+		return -EINVAL;
+
+	msg.req.sw_id = sw_id;
+	msg.req.secondary_sw_id = secondary_sw_id;
+	msg.resp.status = TMEL_ERROR_GENERIC;
+
+	ret = tmel_process_request(tdev, TMEL_MSG_UID_SECBOOT_SS_TEAR_DOWN,
+				   &msg, sizeof(msg));
+	if (ret) {
+		dev_err(dev, "Failed to send IPC: %d\n", ret);
+	} else if (msg.resp.status) {
+		dev_err(dev, "Failed with status: %d\n", msg.resp.status);
+		ret = msg.resp.status ? -EINVAL : 0;
+	}
+
+	return ret;
+}
+
+static inline void tmel_qmp_send_work(struct work_struct *work)
+{
+	struct tmel *tdev = container_of(work, struct tmel, twork);
+	struct tmel_qmp_msg *tmsg = tdev->data;
+	struct tmel_sec_auth *smsg = tmsg->msg;
+	struct mbox_chan *chan;
+	int ret;
+
+	chan = &tdev->ctrl.chans[0];
+
+	switch (tmsg->msg_id) {
+	case TMEL_MSG_UID_SECBOOT_SEC_AUTH:
+		ret = tmel_secboot_sec_auth(tdev, smsg->pas_id, smsg->data, smsg->size);
+		break;
+	case TMEL_MSG_UID_SECBOOT_SS_TEAR_DOWN:
+		ret = tmel_secboot_teardown(tdev, smsg->pas_id, 0);
+		break;
+	}
+
+	mbox_chan_txdone(chan, ret);
+}
+
+/**
+ * tmel_qmp_startup() - Start qmp mailbox channel for communication.
+ * @chan: mailbox channel that is being opened.
+ * Waits for remote subsystem to open channel if link is not
+ * initiated or until timeout.
+ */
+static inline int tmel_qmp_startup(struct mbox_chan *chan)
+{
+	struct tmel *tdev = chan->con_priv;
+	struct qmp_device *mdev = tdev->mdev;
+	unsigned long flags;
+	int ret;
+
+	/*
+	 * Kick start the SM from the negotiation phase
+	 * Rest of the link changes would follow when remote responds.
+	 */
+	spin_lock_irqsave(&mdev->tx_lock, flags);
+	mdev->mcore.bits.link_state = 1;
+	mdev->local_state = LINK_NEGOTIATION;
+	spin_unlock_irqrestore(&mdev->tx_lock, flags);
+
+	mdev->rx_pkt.iov_base = devm_kcalloc(mdev->dev, 1, QMP_MAX_PKT_SIZE,
+					     GFP_KERNEL);
+	if (!mdev->rx_pkt.iov_base)
+		return -ENOMEM;
+
+	tmel_qmp_send_irq(mdev);
+
+	ret = wait_for_completion_timeout(&mdev->link_complete,
+					  msecs_to_jiffies(QMP_TOUT_MS));
+	if (!ret)
+		return -EAGAIN;
+
+	spin_lock_irqsave(&mdev->tx_lock, flags);
+	if (mdev->local_state == LINK_CONNECTED) {
+		mdev->mcore.bits.ch_state = 1;
+		mdev->local_state = LOCAL_CONNECTING;
+		dev_dbg(mdev->dev, "link complete, local connecting");
+		tmel_qmp_send_irq(mdev);
+	}
+	spin_unlock_irqrestore(&mdev->tx_lock, flags);
+
+	ret = wait_for_completion_timeout(&mdev->ch_complete,
+					  msecs_to_jiffies(QMP_TOUT_MS));
+	if (!ret)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+/**
+ * tmel_qmp_shutdown() - Shutdown this mailbox channel.
+ * @chan: mailbox channel to be shutdown.
+ * Disconnect this mailbox channel so the client does not receive anymore
+ * data and can reliquish control of the channel.
+ */
+static inline void tmel_qmp_shutdown(struct mbox_chan *chan)
+{
+	struct qmp_device *mdev = chan->con_priv;
+	unsigned long flags;
+
+	spin_lock_irqsave(&mdev->tx_lock, flags);
+	if (mdev->local_state != LINK_DISCONNECTED) {
+		mdev->local_state = LOCAL_DISCONNECTING;
+		mdev->mcore.bits.ch_state = 0;
+		tmel_qmp_send_irq(mdev);
+	}
+	spin_unlock_irqrestore(&mdev->tx_lock, flags);
+}
+
+static inline int tmel_qmp_send(struct mbox_chan *chan, void *data)
+{
+	struct tmel *tdev = chan->con_priv;
+
+	tdev->data = data;
+	queue_work(system_wq, &tdev->twork);
+
+	return 0;
+}
+
+static struct mbox_chan_ops tmel_qmp_ops = {
+	.startup = tmel_qmp_startup,
+	.shutdown = tmel_qmp_shutdown,
+	.send_data = tmel_qmp_send,
+};
+
+static inline struct tmel *tmel_init(struct platform_device *pdev)
+{
+	struct tmel *tdev;
+	struct mbox_chan *chans;
+
+	tdev = devm_kcalloc(&pdev->dev, 1, sizeof(*tdev), GFP_KERNEL);
+	if (!tdev)
+		return ERR_PTR(-ENOMEM);
+
+	tdev->ipc_pkt = devm_kcalloc(&pdev->dev, 1, sizeof(struct tmel_ipc_pkt), GFP_KERNEL);
+	if (!tdev->ipc_pkt)
+		return ERR_PTR(-ENOMEM);
+
+	init_waitqueue_head(&tdev->waitq);
+
+	tdev->rx_done = false;
+	tdev->dev = &pdev->dev;
+	platform_set_drvdata(pdev, tdev);
+
+	chans = devm_kcalloc(&pdev->dev, QMP_NUM_CHANS, sizeof(*chans), GFP_KERNEL);
+	if (!chans)
+		return ERR_PTR(-ENOMEM);
+
+	tdev->ctrl.chans = chans;
+	INIT_WORK(&tdev->twork, tmel_qmp_send_work);
+
+	return tdev;
+}
+
+static inline struct qmp_device *qmp_init(struct platform_device *pdev)
+{
+	struct qmp_device *mdev;
+
+	mdev = devm_kcalloc(&pdev->dev, 1, sizeof(*mdev), GFP_KERNEL);
+	if (!mdev)
+		return ERR_PTR(-ENOMEM);
+
+	mdev->dev = &pdev->dev;
+	mdev->mcore_desc = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mdev->mcore_desc))
+		return ERR_PTR(-EIO);
+
+	mdev->ucore_desc = mdev->mcore_desc + QMP_UCORE_DESC_OFFSET;
+
+	spin_lock_init(&mdev->tx_lock);
+	mdev->local_state = LINK_DISCONNECTED;
+	init_completion(&mdev->link_complete);
+	init_completion(&mdev->ch_complete);
+
+	return mdev;
+}
+
+static inline int qmp_mbox_client_init(struct qmp_device *mdev)
+{
+	int ret = 0;
+
+	mdev->mbox_client.dev = mdev->dev;
+	mdev->mbox_client.knows_txdone = false;
+	mdev->mbox_chan = mbox_request_channel(&mdev->mbox_client, 0);
+	if (IS_ERR(mdev->mbox_chan))
+		ret = PTR_ERR(mdev->mbox_chan);
+
+	return ret;
+}
+
+static inline int tmel_mbox_ctrl_init(struct tmel *tdev)
+{
+	tdev->ctrl.dev = tdev->dev;
+	tdev->ctrl.ops = &tmel_qmp_ops;
+	tdev->ctrl.chans[0].con_priv = tdev;
+	tdev->ctrl.num_chans = QMP_NUM_CHANS;
+	tdev->ctrl.txdone_irq = true;
+
+	return devm_mbox_controller_register(tdev->dev, &tdev->ctrl);
+}
+
+static inline int tmel_qmp_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct qmp_device *mdev;
+	struct tmel *tdev;
+	int ret = 0;
+
+	tdev = tmel_init(pdev);
+	if (IS_ERR(tdev))
+		return dev_err_probe(tdev->dev, ret, "tmel device init failed\n");
+
+	mdev = qmp_init(pdev);
+	if (IS_ERR(mdev))
+		return dev_err_probe(tdev->dev, ret, "qmp device init failed\n");
+
+	tdev->mdev = mdev;
+
+	ret = qmp_mbox_client_init(mdev);
+	if (ret)
+		return dev_err_probe(mdev->dev, ret, "IPC chan missing, client init failed");
+
+	ret = tmel_mbox_ctrl_init(tdev);
+	if (ret)
+		return dev_err_probe(tdev->dev, ret, "failed to register mbox controller");
+
+	ret = platform_get_irq(pdev, 0);
+	ret = devm_request_threaded_irq(tdev->dev, ret, NULL, tmel_qmp_irq_handler,
+					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+					node->name, (void *)tdev);
+	if (ret < 0)
+		return dev_err_probe(tdev->dev, ret, "request threaded irq failed\n");
+
+	return ret;
+}
+
+static const struct of_device_id tmel_qmp_dt_match[] = {
+	{ .compatible = "qcom,ipq5424-tmel" },
+	{},
+};
+
+static struct platform_driver tmel_qmp_driver = {
+	.driver = {
+		.name = "tmel_qmp_mbox",
+		.of_match_table = tmel_qmp_dt_match,
+	},
+	.probe = tmel_qmp_probe,
+};
+module_platform_driver(tmel_qmp_driver);
+
+MODULE_DESCRIPTION("QCOM TMEL QMP driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mailbox/tmelcom-qmp.h b/include/linux/mailbox/tmelcom-qmp.h
new file mode 100644
index 000000000000..338fdea1e37e
--- /dev/null
+++ b/include/linux/mailbox/tmelcom-qmp.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+#ifndef _TMELCOM_H_
+#define _TMELCOM_H_
+
+#include <linux/bitfield.h>
+
+/*
+ * Macro used to define unique TMEL Message Identifier based on
+ * message type and action identifier.
+ */
+#define MSGTYPE_MASK GENMASK(15, 8)
+#define ACTIONID_MASK GENMASK(7, 0)
+
+#define TMEL_MSG_UID_CREATE(msg_type, action_id) \
+	(FIELD_PREP_CONST(MSGTYPE_MASK, msg_type) | \
+	FIELD_PREP_CONST(ACTIONID_MASK, action_id))
+
+/*
+ * Helper macro to extract the messageType from TMEL_MSG_UID
+ */
+#define TMEL_MSG_UID_MSG_TYPE(v)	FIELD_GET(MSGTYPE_MASK, v)
+
+/*
+ * Helper macro to extract the actionID from TMEL_MSG_UID
+ */
+#define TMEL_MSG_UID_ACTION_ID(v)	FIELD_GET(ACTIONID_MASK, v)
+
+/*
+ * All definitions of supported messageTypes.
+ */
+#define TMEL_MSG_SECBOOT	0x00
+
+/*
+ * Action IDs for TMEL_MSG_SECBOOT
+ */
+#define TMEL_ACTION_SECBOOT_SEC_AUTH		0x04
+#define TMEL_ACTION_SECBOOT_SS_TEAR_DOWN	0x0a
+
+/*
+ * UIDs for TMEL_MSG_SECBOOT
+ */
+#define TMEL_MSG_UID_SECBOOT_SEC_AUTH	    TMEL_MSG_UID_CREATE(TMEL_MSG_SECBOOT,\
+					    TMEL_ACTION_SECBOOT_SEC_AUTH)
+
+#define TMEL_MSG_UID_SECBOOT_SS_TEAR_DOWN	TMEL_MSG_UID_CREATE(TMEL_MSG_SECBOOT,\
+						TMEL_ACTION_SECBOOT_SS_TEAR_DOWN)
+
+struct tmel_qmp_msg {
+	void *msg;
+	u32 msg_id;
+};
+
+struct tmel_sec_auth {
+	void *data;
+	u32 size;
+	u32 pas_id;
+};
+
+void tmel_secboot_sec_free(void *ptr);
+
+DEFINE_FREE(tmel_secboot_sec_f, void *, if (_T) tmel_secboot_sec_free(_T))
+#endif  /* _TMELCOM_H_ */
-- 
2.34.1

Re: [PATCH V4 1/2] dt-bindings: mailbox: Document qcom,ipq5424-tmel

[Krzysztof Kozlowski]

On Thu, Mar 27, 2025 at 11:47:49PM +0530, Sricharan R wrote:
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - qcom,ipq5424-tmel

blank line

> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  mboxes:
> +    maxItems: 1

Why mbox is having an mbox? This does not look right and suggest the
block is misrepresented. I read the diagram and description two times
and still do not see how this fits there.

> +
> +  "#mbox-cells":
> +    const: 1

Best regards,
Krzysztof

Re: [PATCH V4 1/2] dt-bindings: mailbox: Document qcom,ipq5424-tmel

[Dmitry Baryshkov]

On Thu, Mar 27, 2025 at 11:47:49PM +0530, Sricharan R wrote:
> From: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> 
> TMEL(Trust Management Engine Lite) subsystem provides different kinds of

Trust whatever SubSystem (TMEL SS) ...

different to what?

> services like secureboot, remote image authentication, key management,
> crypto, OEM provisioning etc.
> 
> The QMP mailbox is the primary means of communication between TMEL SS and

What is QMP?

> other subsystem on the SoC. A dedicated pair of inbound and outbound
> mailboxes is implemented for each subsystem/external execution environment

Is it implemented in the driver? Is it provided by the hardware? By the
firmware?

> which needs to communicate with TMEL for security services. The inbound
> mailboxes are used to send IPC requests to TMEL, which are then processed
> by TMEL firmware and accordingly the responses are sent back via outbound
> mailboxes.

-- 
With best wishes
Dmitry

Re: [PATCH V4 1/2] dt-bindings: mailbox: Document qcom,ipq5424-tmel

[Sricharan Ramabadhran]

On 3/28/2025 1:32 PM, Krzysztof Kozlowski wrote:
> On Thu, Mar 27, 2025 at 11:47:49PM +0530, Sricharan R wrote:
>> +properties:
>> +  compatible:
>> +    items:
>> +      - enum:
>> +          - qcom,ipq5424-tmel
> 
> blank line
ok

> 
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  mboxes:
>> +    maxItems: 1
> 
> Why mbox is having an mbox? This does not look right and suggest the
> block is misrepresented. I read the diagram and description two times
> and still do not see how this fits there.
TMEL/QMP secure functionalities are exposed to clients (like rproc) by 
registering TMEL as mailbox controller. The IPC bit to communicate with 
the TMEL/QMP controller itself is handled by the apcs mailbox. Hence
it looks like a mbox inside mbox.

Alternatively, would it be fine to fold the IPC bit handling in this 
driver itself for doing the regmap_write (instead of connecting to
apcs mailbox separately) ?

Also, there are couple of other ways of designing this as well.
Earlier i mentioned this in the RFC post [1] for getting the design
sorted out.

[1] 
https://lore.kernel.org/lkml/20241205080633.2623142-1-quic_srichara@quicinc.com/T/

-------------------------------------------------------------
Had the below mentioned in the RFC earlier.

The intention of posting this is to get the design reviewed/corrected 
since there are also other possible ways of having this SS support like:

a) Make TMEL QMP as a 'rpmsg' driver and clients can connect using
    rmpsg_send

b) Keep TMEL APIs seperately in drivers/firmware which would export APIs
    and QMP mailbox seperately.
    Clients can then call the exported APIS.

c) Combine both TMEL and QMP as mailbox (this is the approach used here)

Regards,
  Sricharan

Re: [PATCH V4 1/2] dt-bindings: mailbox: Document qcom,ipq5424-tmel

[Sricharan Ramabadhran]

On 3/28/2025 6:21 PM, Dmitry Baryshkov wrote:
> On Thu, Mar 27, 2025 at 11:47:49PM +0530, Sricharan R wrote:
>> From: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>>
>> TMEL(Trust Management Engine Lite) subsystem provides different kinds of
> 
> Trust whatever SubSystem (TMEL SS) ...
> 
> different to what?
> To the ARM TrustZone firmware(TZ). So these services (secureboot, image
authentication etc) were provided by the TZ in some SOCs. Here, TMEL
provides those. Can add those details here.

>> services like secureboot, remote image authentication, key management,
>> crypto, OEM provisioning etc.
>>
>> The QMP mailbox is the primary means of communication between TMEL SS and
> 
> What is QMP?
Qualcomm Messaging Protocol

> 
>> other subsystem on the SoC. A dedicated pair of inbound and outbound
>> mailboxes is implemented for each subsystem/external execution environment
> 
> Is it implemented in the driver? Is it provided by the hardware? By the
> firmware?
>
TMEL firmware provides and processes the inbound requests and responds
back on the outbound channel. Can mention this explicitly in the above.

Regards,
  Sricharan

Re: [PATCH V4 1/2] dt-bindings: mailbox: Document qcom,ipq5424-tmel

[Sricharan Ramabadhran]

[Resending, since my previous response had some wrapping issue]

>> TMEL(Trust Management Engine Lite) subsystem provides different kinds of
> 
> Trust whatever SubSystem (TMEL SS) ...
> 
> different to what?


To the ARM TrustZone firmware(TZ). So these services (secureboot,
authentication etc) were provided by the TZ in some SOCs. Here,
TMEL provides those. Can add those details here.

> 
>> services like secureboot, remote image authentication, key management,
>> crypto, OEM provisioning etc.
>>
>> The QMP mailbox is the primary means of communication between TMEL SS and
> 
> What is QMP?
Qualcomm Messaging Protocol

> 
>> other subsystem on the SoC. A dedicated pair of inbound and outbound
>> mailboxes is implemented for each subsystem/external execution environment
> 
> Is it implemented in the driver? Is it provided by the hardware? By the
> firmware?
> 
TMEL firmware provides and processes the inbound requests and responds
back on the outbound channel. Can mention this explicitly in the above.

Regards,
  Sricharan

Re: [PATCH V4 1/2] dt-bindings: mailbox: Document qcom,ipq5424-tmel

[Dmitry Baryshkov]

On 01/04/2025 14:26, Sricharan Ramabadhran wrote:
> 
> [Resending, since my previous response had some wrapping issue]
> 
>>> TMEL(Trust Management Engine Lite) subsystem provides different kinds of
>>
>> Trust whatever SubSystem (TMEL SS) ...
>>
>> different to what?
> 
> 
> To the ARM TrustZone firmware(TZ). So these services (secureboot,
> authentication etc) were provided by the TZ in some SOCs. Here,
> TMEL provides those. Can add those details here.

Yes, please (and all other answers too).

> 
>>
>>> services like secureboot, remote image authentication, key management,
>>> crypto, OEM provisioning etc.
>>>
>>> The QMP mailbox is the primary means of communication between TMEL SS 
>>> and
>>
>> What is QMP?
> Qualcomm Messaging Protocol
> 
>>
>>> other subsystem on the SoC. A dedicated pair of inbound and outbound
>>> mailboxes is implemented for each subsystem/external execution 
>>> environment
>>
>> Is it implemented in the driver? Is it provided by the hardware? By the
>> firmware?
>>
> TMEL firmware provides and processes the inbound requests and responds
> back on the outbound channel. Can mention this explicitly in the above.
> 
> Regards,
>   Sricharan


-- 
With best wishes
Dmitry

Re: [PATCH V4 1/2] dt-bindings: mailbox: Document qcom,ipq5424-tmel

[Sricharan Ramabadhran]

On 4/1/2025 12:59 PM, Sricharan Ramabadhran wrote:
> 
> 
> On 3/28/2025 1:32 PM, Krzysztof Kozlowski wrote:
>> On Thu, Mar 27, 2025 at 11:47:49PM +0530, Sricharan R wrote:
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - qcom,ipq5424-tmel
>>
>> blank line
> ok
> 
>>
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  mboxes:
>>> +    maxItems: 1
>>
>> Why mbox is having an mbox? This does not look right and suggest the
>> block is misrepresented. I read the diagram and description two times
>> and still do not see how this fits there.
> TMEL/QMP secure functionalities are exposed to clients (like rproc) by 
> registering TMEL as mailbox controller. The IPC bit to communicate with 
> the TMEL/QMP controller itself is handled by the apcs mailbox. Hence
> it looks like a mbox inside mbox.
> 
> Alternatively, would it be fine to fold the IPC bit handling in this 
> driver itself for doing the regmap_write (instead of connecting to
> apcs mailbox separately) ?
> 
> Also, there are couple of other ways of designing this as well.
> Earlier i mentioned this in the RFC post [1] for getting the design
> sorted out.
> 
> [1] https://lore.kernel.org/lkml/20241205080633.2623142-1- 
> quic_srichara@quicinc.com/T/
> 
> -------------------------------------------------------------
> Had the below mentioned in the RFC earlier.
> 
> The intention of posting this is to get the design reviewed/corrected 
> since there are also other possible ways of having this SS support like:
> 
> a) Make TMEL QMP as a 'rpmsg' driver and clients can connect using
>     rmpsg_send
> 
> b) Keep TMEL APIs seperately in drivers/firmware which would export APIs
>     and QMP mailbox seperately.
>     Clients can then call the exported APIS.
> 
> c) Combine both TMEL and QMP as mailbox (this is the approach used here)
> 

Hi Krysztof,

Can you kindly provide your suggestion on how to proceed for the above ?
Would want to align on the design approach before posting the next
version.

Regards,
  Sricharan

Re: [PATCH V4 2/2] mailbox: tmelite-qmp: Introduce TMEL QMP mailbox driver

[Konrad Dybcio]

On 3/27/25 7:17 PM, Sricharan R wrote:
> From: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> 
> This mailbox facilitates the communication between the TMEL server
> subsystem (Trust Management Engine Lite) and the TMEL client
> (APPSS/BTSS/AUDIOSS), used for secure services like secure image
> authentication, enable/disable efuses, crypto services etc. Each client in
> the SoC has its own block of message RAM and IRQ for communication with the
> TMEL SS. The protocol used to communicate in the message RAM is known as
> Qualcomm Messaging Protocol (QMP).
> 
> Remote proc driver subscribes to this mailbox and uses the
> mbox_send_message to use TMEL to securely authenticate/teardown the images.
> 
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> ---

[...]

> +/*
> + * mbox data can be shared over mem or sram
> + */

/* foo */

[...]

> +enum ipc_type {
> +	IPC_MBOX_MEM,
> +	IPC_MBOX_SRAM,
> +};
> +
> +/*
> + * mbox header indicates the type of payload and action required.
> + */
> +struct ipc_header {
> +	u8 ipc_type:1;
> +	u8 msg_len:7;
> +	u8 msg_type;
> +	u8 action_id;
> +	s8 response;
> +};

You said in the changelog that __packed is not required.. I suppose it's
technically correct, but I think it's good practice to add it on anything
that's bounced between blocks and is of fixed size

[...]

> +/**
> + * tmel_qmp_send_irq() - send an irq to a remote entity as an event signal.
> + * @mdev: Which remote entity that should receive the irq.
> + */
> +static inline void tmel_qmp_send_irq(struct qmp_device *mdev)
> +{
> +	writel(mdev->mcore.val, mdev->mcore_desc);
> +	/* Ensure desc update is visible before IPC */
> +	wmb();

writel (non _relaxed) already includes a write barrier, to ensure write
observability at the other endpoint, you'd have to read back the register
instead

> +
> +	dev_dbg(mdev->dev, "%s: mcore 0x%x ucore 0x%x", __func__,
> +		mdev->mcore.val, mdev->ucore.val);
> +
> +	mbox_send_message(mdev->mbox_chan, NULL);
> +	mbox_client_txdone(mdev->mbox_chan, 0);
> +}
> +
> +/**
> + * tmel_qmp_send_data() - Send the data to remote and notify.
> + * @mdev: qmp_device to send the data to.
> + * @data: Data to be sent to remote processor, should be in the format of
> + *	  a kvec.
> + *
> + * Copy the data to the channel's mailbox and notify remote subsystem of new
> + * data. This function will return an error if the previous message sent has
> + * not been read.

This is not valid kerneldoc, check make W=1, there are many cases in
this file

[...]

> +	/* read remote_desc from mailbox register */

All other comments start with an uppercase letter, please make it
consistent

[...]

> +	mdev->ucore.val = readl(mdev->ucore_desc);
> +
> +	dev_dbg(mdev->dev, "%s: mcore 0x%x ucore 0x%x", __func__,
> +		mdev->mcore.val, mdev->ucore.val);
> +
> +	spin_lock_irqsave(&mdev->tx_lock, flags);
> +
> +	/* Check if remote link down */
> +	if (mdev->local_state >= LINK_CONNECTED &&
> +	    !(mdev->ucore.bits.link_state)) {
> +		mdev->local_state = LINK_NEGOTIATION;
> +		mdev->mcore.bits.link_state_ack = mdev->ucore.bits.link_state;

You dereference into local_state, mcore and ucore a lot - consider
creating a variable to reduce the constant ->/.-age

[...]

> +	if (sizeof(struct ipc_header) + msg_size <= QMP_MBOX_IPC_PACKET_SIZE) {
> +		/* Mbox only */
> +		msg_hdr->ipc_type = IPC_MBOX_MEM;
> +		msg_hdr->msg_len = msg_size;
> +		memcpy((void *)mbox_payload, msg_buf, msg_size);
> +	} else if (msg_size <= QMP_SRAM_IPC_MAX_BUF_SIZE) {

> +		/* SRAM */
> +		msg_hdr->ipc_type = IPC_MBOX_SRAM;
> +		msg_hdr->msg_len = 8;

I think we should check for == and not <= to rule out some partially
transacted messages

[...]

> +
> +		tdev->sram_dma_addr = dma_map_single(tdev->dev, msg_buf,
> +						     msg_size,
> +						     DMA_BIDIRECTIONAL);
> +		ret = dma_mapping_error(tdev->dev, tdev->sram_dma_addr);
> +		if (ret) {
> +			dev_err(tdev->dev, "SRAM DMA mapping error: %d\n", ret);
> +			return ret;
> +		}
> +
> +		sram_payload->payload_ptr = tdev->sram_dma_addr;
> +		sram_payload->payload_len = msg_size;
> +	} else {
> +		dev_err(tdev->dev, "Invalid payload length: %zu\n", msg_size);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * tmel_unprepare_message() - Get the response data back for client
> + * @tdev: the tmel device
> + * @msg_buf: payload to be sent
> + * @msg_size: size of the payload
> + */
> +static inline void tmel_unprepare_message(struct tmel *tdev, void *msg_buf, size_t msg_size)
> +{
> +	struct tmel_ipc_pkt *ipc_pkt = (struct tmel_ipc_pkt *)tdev->pkt.iov_base;
> +	struct mbox_payload *mbox_payload = &ipc_pkt->payload.mbox_payload;
> +
> +	if (ipc_pkt->msg_hdr.ipc_type == IPC_MBOX_MEM) {
> +		memcpy(msg_buf, mbox_payload, msg_size);
> +	} else if (ipc_pkt->msg_hdr.ipc_type == IPC_MBOX_SRAM) {
> +		dma_unmap_single(tdev->dev, tdev->sram_dma_addr, msg_size, DMA_BIDIRECTIONAL);
> +		tdev->sram_dma_addr = 0;
> +	}
> +}
> +
> +static inline bool tmel_rx_done(struct tmel *tdev)
> +{
> +	return tdev->rx_done;
> +}
> +
> +/**
> + * tmel_process_request() - process client msg and wait for response
> + * @tdev: the tmel device
> + * @msg_uid: msg_type/action_id combo
> + * @msg_buf: payload to be sent
> + * @msg_size: size of the payload
> + */
> +static inline int tmel_process_request(struct tmel *tdev, u32 msg_uid,
> +				       void *msg_buf, size_t msg_size)
> +{
> +	struct qmp_device *mdev = tdev->mdev;
> +	struct tmel_ipc_pkt *resp_ipc_pkt;
> +	struct mbox_chan *chan;
> +	unsigned long jiffies;
> +	long time_left = 0;
> +	int ret = 0;
> +
> +	chan = &tdev->ctrl.chans[0];
> +
> +	if (!msg_buf || !msg_size) {
> +		dev_err(tdev->dev, "Invalid msg_buf or msg_size\n");
> +		return -EINVAL;
> +	}
> +
> +	tdev->rx_done = false;
> +
> +	ret = tmel_prepare_msg(tdev, msg_uid, msg_buf, msg_size);
> +	if (ret)
> +		return ret;
> +
> +	tdev->pkt.iov_len = sizeof(struct tmel_ipc_pkt);
> +	tdev->pkt.iov_base = (void *)tdev->ipc_pkt;
> +
> +	tmel_qmp_send_data(mdev, &tdev->pkt);
> +	jiffies = msecs_to_jiffies(QMP_SEND_TIMEOUT);
> +
> +	time_left = wait_event_interruptible_timeout(tdev->waitq,
> +						     tmel_rx_done(tdev),
> +						     jiffies);
> +

Unexpected \n

[...]

> +	if (ret) {
> +		dev_err(dev, "Failed to send IPC: %d\n", ret);
> +	} else if (smsg->msg.resp.status) {
> +		dev_err(dev, "Failed with status: %d", smsg->msg.resp.status);
> +		ret = smsg->msg.resp.status ? -EINVAL : 0;

Do the internal error numbers correspond to linux errnos?

E.g. is there an TMEL_ERROR_TIMEDOUT that could be mapped to
ETIMEDOUT?

[...]

> +	/*
> +	 * Kick start the SM from the negotiation phase

Please spell out state machine, it's not obvious

[...]

> +
> +	ret = platform_get_irq(pdev, 0);

This return value is never checked

Konrad

Re: [PATCH V4 2/2] mailbox: tmelite-qmp: Introduce TMEL QMP mailbox driver

[Konrad Dybcio]

On 3/27/25 7:17 PM, Sricharan R wrote:
> From: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> 
> This mailbox facilitates the communication between the TMEL server
> subsystem (Trust Management Engine Lite) and the TMEL client
> (APPSS/BTSS/AUDIOSS), used for secure services like secure image
> authentication, enable/disable efuses, crypto services etc. Each client in
> the SoC has its own block of message RAM and IRQ for communication with the
> TMEL SS. The protocol used to communicate in the message RAM is known as
> Qualcomm Messaging Protocol (QMP).
> 
> Remote proc driver subscribes to this mailbox and uses the
> mbox_send_message to use TMEL to securely authenticate/teardown the images.
> 
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> ---

[...]

> +
> +#define QMP_NUM_CHANS		0x1

Quantities make more sense in decimal, but since this is effectively
a single-use value, you can put in the '1' literal in num_chans and use
devm_kzalloc instead of devm_kcalloc in the other use

> +#define QMP_TOUT_MS		1000

"TIMEOUT"

> +#define QMP_CTRL_DATA_SIZE	4
> +#define QMP_MAX_PKT_SIZE	0x18

This is very handwavy, please structurize all data that comes in and
out of the mailbox.

> +#define QMP_UCORE_DESC_OFFSET	0x1000
> +#define QMP_SEND_TIMEOUT	30000

Please include the unit in the macro name - although 30s is quite a
timeout for a couple bytes..

[...]

> +#define QMP_HW_MBOX_SIZE		32
> +#define QMP_MBOX_RSV_SIZE		4
> +#define QMP_MBOX_IPC_PACKET_SIZE	(QMP_HW_MBOX_SIZE - QMP_CTRL_DATA_SIZE - QMP_MBOX_RSV_SIZE)
> +#define QMP_MBOX_IPC_MAX_PARAMS		5
> +
> +#define QMP_MAX_PARAM_IN_PARAM_ID	14
> +#define QMP_PARAM_CNT_FOR_OUTBUF	3
> +#define QMP_SRAM_IPC_MAX_PARAMS		(QMP_MAX_PARAM_IN_PARAM_ID * QMP_PARAM_CNT_FOR_OUTBUF)
> +#define QMP_SRAM_IPC_MAX_BUF_SIZE	(QMP_SRAM_IPC_MAX_PARAMS * sizeof(u32))

These should be expressed in terms of structures and sizeof() instead,
as well

> +
> +#define TMEL_ERROR_GENERIC		(0x1u)
> +#define TMEL_ERROR_NOT_SUPPORTED	(0x2u)
> +#define TMEL_ERROR_BAD_PARAMETER	(0x3u)
> +#define TMEL_ERROR_BAD_MESSAGE		(0x4u)
> +#define TMEL_ERROR_BAD_ADDRESS		(0x5u)
> +#define TMEL_ERROR_TMELCOM_FAILURE	(0x6u)
> +#define TMEL_ERROR_TMEL_BUSY		(0x7u)

Oh I didn't notice this during the first review.. I assume these are
returned by the mbox. Please create a dictionary such as:

u32 tmel_error_dict[] = {
	[TMEL_ERROR_GENERIC] = EINVAL,
	[TMEL_ERROR_NOT_SUPPORTED] = EOPNOTSUPP
	...
};

that we can then plug into the function down below that currently does
error ? -EINVAL : 0

Konrad

Re: [PATCH V4 2/2] mailbox: tmelite-qmp: Introduce TMEL QMP mailbox driver

[Sricharan Ramabadhran]

On 4/26/2025 3:14 PM, Konrad Dybcio wrote:
> On 3/27/25 7:17 PM, Sricharan R wrote:
>> From: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>>
>> This mailbox facilitates the communication between the TMEL server
>> subsystem (Trust Management Engine Lite) and the TMEL client
>> (APPSS/BTSS/AUDIOSS), used for secure services like secure image
>> authentication, enable/disable efuses, crypto services etc. Each client in
>> the SoC has its own block of message RAM and IRQ for communication with the
>> TMEL SS. The protocol used to communicate in the message RAM is known as
>> Qualcomm Messaging Protocol (QMP).
>>
>> Remote proc driver subscribes to this mailbox and uses the
>> mbox_send_message to use TMEL to securely authenticate/teardown the images.
>>
>> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>> ---
> 
> [...]
> 
>> +/*
>> + * mbox data can be shared over mem or sram
>> + */
> 
> /* foo */
> 
ok.

> [...]
> 
>> +enum ipc_type {
>> +	IPC_MBOX_MEM,
>> +	IPC_MBOX_SRAM,
>> +};
>> +
>> +/*
>> + * mbox header indicates the type of payload and action required.
>> + */
>> +struct ipc_header {
>> +	u8 ipc_type:1;
>> +	u8 msg_len:7;
>> +	u8 msg_type;
>> +	u8 action_id;
>> +	s8 response;
>> +};
> 
> You said in the changelog that __packed is not required.. I suppose it's
> technically correct, but I think it's good practice to add it on anything
> that's bounced between blocks and is of fixed size
> 
ok.

> [...]
> 
>> +/**
>> + * tmel_qmp_send_irq() - send an irq to a remote entity as an event signal.
>> + * @mdev: Which remote entity that should receive the irq.
>> + */
>> +static inline void tmel_qmp_send_irq(struct qmp_device *mdev)
>> +{
>> +	writel(mdev->mcore.val, mdev->mcore_desc);
>> +	/* Ensure desc update is visible before IPC */
>> +	wmb();
> 
> writel (non _relaxed) already includes a write barrier, to ensure write
> observability at the other endpoint, you'd have to read back the register
> instead
> 
ok

>> +
>> +	dev_dbg(mdev->dev, "%s: mcore 0x%x ucore 0x%x", __func__,
>> +		mdev->mcore.val, mdev->ucore.val);
>> +
>> +	mbox_send_message(mdev->mbox_chan, NULL);
>> +	mbox_client_txdone(mdev->mbox_chan, 0);
>> +}
>> +  
>> +/**
>> + * tmel_qmp_send_data() - Send the data to remote and notify.
>> + * @mdev: qmp_device to send the data to.
>> + * @data: Data to be sent to remote processor, should be in the format of
>> + *	  a kvec.
>> + *
>> + * Copy the data to the channel's mailbox and notify remote subsystem of new
>> + * data. This function will return an error if the previous message sent has
>> + * not been read.
> 
> This is not valid kerneldoc, check make W=1, there are many cases in
> this file
> 
ok, but i ran kerneldoc check and did not get any error. Anyways let me
re-check with W=1 once.

> [...]
> 
>> +	/* read remote_desc from mailbox register */
> 
> All other comments start with an uppercase letter, please make it
> consistent
> 
ok.

> [...]
> 
>> +	mdev->ucore.val = readl(mdev->ucore_desc);
>> +
>> +	dev_dbg(mdev->dev, "%s: mcore 0x%x ucore 0x%x", __func__,
>> +		mdev->mcore.val, mdev->ucore.val);
>> +
>> +	spin_lock_irqsave(&mdev->tx_lock, flags);
>> +
>> +	/* Check if remote link down */
>> +	if (mdev->local_state >= LINK_CONNECTED &&
>> +	    !(mdev->ucore.bits.link_state)) {
>> +		mdev->local_state = LINK_NEGOTIATION;
>> +		mdev->mcore.bits.link_state_ack = mdev->ucore.bits.link_state;
> 
> You dereference into local_state, mcore and ucore a lot - consider
> creating a variable to reduce the constant ->/.-age
> 
ok

> [...]
> 
>> +	if (sizeof(struct ipc_header) + msg_size <= QMP_MBOX_IPC_PACKET_SIZE) {
>> +		/* Mbox only */
>> +		msg_hdr->ipc_type = IPC_MBOX_MEM;
>> +		msg_hdr->msg_len = msg_size;
>> +		memcpy((void *)mbox_payload, msg_buf, msg_size);
>> +	} else if (msg_size <= QMP_SRAM_IPC_MAX_BUF_SIZE) {
> 
>> +		/* SRAM */
>> +		msg_hdr->ipc_type = IPC_MBOX_SRAM;
>> +		msg_hdr->msg_len = 8;
> 
> I think we should check for == and not <= to rule out some partially
> transacted messages
> 
ok, will fix.

> [...]
> 
>> +
>> +		tdev->sram_dma_addr = dma_map_single(tdev->dev, msg_buf,
>> +						     msg_size,
>> +						     DMA_BIDIRECTIONAL);
>> +		ret = dma_mapping_error(tdev->dev, tdev->sram_dma_addr);
>> +		if (ret) {
>> +			dev_err(tdev->dev, "SRAM DMA mapping error: %d\n", ret);
>> +			return ret;
>> +		}
>> +
>> +		sram_payload->payload_ptr = tdev->sram_dma_addr;
>> +		sram_payload->payload_len = msg_size;
>> +	} else {
>> +		dev_err(tdev->dev, "Invalid payload length: %zu\n", msg_size);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * tmel_unprepare_message() - Get the response data back for client
>> + * @tdev: the tmel device
>> + * @msg_buf: payload to be sent
>> + * @msg_size: size of the payload
>> + */
>> +static inline void tmel_unprepare_message(struct tmel *tdev, void *msg_buf, size_t msg_size)
>> +{
>> +	struct tmel_ipc_pkt *ipc_pkt = (struct tmel_ipc_pkt *)tdev->pkt.iov_base;
>> +	struct mbox_payload *mbox_payload = &ipc_pkt->payload.mbox_payload;
>> +
>> +	if (ipc_pkt->msg_hdr.ipc_type == IPC_MBOX_MEM) {
>> +		memcpy(msg_buf, mbox_payload, msg_size);
>> +	} else if (ipc_pkt->msg_hdr.ipc_type == IPC_MBOX_SRAM) {
>> +		dma_unmap_single(tdev->dev, tdev->sram_dma_addr, msg_size, DMA_BIDIRECTIONAL);
>> +		tdev->sram_dma_addr = 0;
>> +	}
>> +}
>> +
>> +static inline bool tmel_rx_done(struct tmel *tdev)
>> +{
>> +	return tdev->rx_done;
>> +}
>> +
>> +/**
>> + * tmel_process_request() - process client msg and wait for response
>> + * @tdev: the tmel device
>> + * @msg_uid: msg_type/action_id combo
>> + * @msg_buf: payload to be sent
>> + * @msg_size: size of the payload
>> + */
>> +static inline int tmel_process_request(struct tmel *tdev, u32 msg_uid,
>> +				       void *msg_buf, size_t msg_size)
>> +{
>> +	struct qmp_device *mdev = tdev->mdev;
>> +	struct tmel_ipc_pkt *resp_ipc_pkt;
>> +	struct mbox_chan *chan;
>> +	unsigned long jiffies;
>> +	long time_left = 0;
>> +	int ret = 0;
>> +
>> +	chan = &tdev->ctrl.chans[0];
>> +
>> +	if (!msg_buf || !msg_size) {
>> +		dev_err(tdev->dev, "Invalid msg_buf or msg_size\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	tdev->rx_done = false;
>> +
>> +	ret = tmel_prepare_msg(tdev, msg_uid, msg_buf, msg_size);
>> +	if (ret)
>> +		return ret;
>> +
>> +	tdev->pkt.iov_len = sizeof(struct tmel_ipc_pkt);
>> +	tdev->pkt.iov_base = (void *)tdev->ipc_pkt;
>> +
>> +	tmel_qmp_send_data(mdev, &tdev->pkt);
>> +	jiffies = msecs_to_jiffies(QMP_SEND_TIMEOUT);
>> +
>> +	time_left = wait_event_interruptible_timeout(tdev->waitq,
>> +						     tmel_rx_done(tdev),
>> +						     jiffies);
>> +
> 
> Unexpected \n
> 
ok.

> [...]
> 
>> +	if (ret) {
>> +		dev_err(dev, "Failed to send IPC: %d\n", ret);
>> +	} else if (smsg->msg.resp.status) {
>> +		dev_err(dev, "Failed with status: %d", smsg->msg.resp.status);
>> +		ret = smsg->msg.resp.status ? -EINVAL : 0;
> 
> Do the internal error numbers correspond to linux errnos?
> 
> E.g. is there an TMEL_ERROR_TIMEDOUT that could be mapped to
> ETIMEDOUT?
> 
There is no direct mapping, hence this sideband conversion.

> [...]
> 
>> +	/*
>> +	 * Kick start the SM from the negotiation phase
> 
> Please spell out state machine, it's not obvious
> 
ok.

> [...]
> 
>> +
>> +	ret = platform_get_irq(pdev, 0);
> 
> This return value is never checked
> 
ok, will fix.

Regards,
  Sricharan

Re: [PATCH V4 2/2] mailbox: tmelite-qmp: Introduce TMEL QMP mailbox driver

[Sricharan Ramabadhran]

On 4/26/2025 3:19 PM, Konrad Dybcio wrote:
> On 3/27/25 7:17 PM, Sricharan R wrote:
>> From: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>>
>> This mailbox facilitates the communication between the TMEL server
>> subsystem (Trust Management Engine Lite) and the TMEL client
>> (APPSS/BTSS/AUDIOSS), used for secure services like secure image
>> authentication, enable/disable efuses, crypto services etc. Each client in
>> the SoC has its own block of message RAM and IRQ for communication with the
>> TMEL SS. The protocol used to communicate in the message RAM is known as
>> Qualcomm Messaging Protocol (QMP).
>>
>> Remote proc driver subscribes to this mailbox and uses the
>> mbox_send_message to use TMEL to securely authenticate/teardown the images.
>>
>> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>> ---
> 
> [...]
> 
>> +
>> +#define QMP_NUM_CHANS		0x1
> 
> Quantities make more sense in decimal, but since this is effectively
> a single-use value, you can put in the '1' literal in num_chans and use
> devm_kzalloc instead of devm_kcalloc in the other use
> 
ok

>> +#define QMP_TOUT_MS		1000
> 
> "TIMEOUT"
> 
ok

>> +#define QMP_CTRL_DATA_SIZE	4
>> +#define QMP_MAX_PKT_SIZE	0x18
> 
> This is very handwavy, please structurize all data that comes in and
> out of the mailbox.
> 
ok

>> +#define QMP_UCORE_DESC_OFFSET	0x1000
>> +#define QMP_SEND_TIMEOUT	30000
> 
> Please include the unit in the macro name - although 30s is quite a
> timeout for a couple bytes..
> 
ok

> [...]
> 
>> +#define QMP_HW_MBOX_SIZE		32
>> +#define QMP_MBOX_RSV_SIZE		4
>> +#define QMP_MBOX_IPC_PACKET_SIZE	(QMP_HW_MBOX_SIZE - QMP_CTRL_DATA_SIZE - QMP_MBOX_RSV_SIZE)
>> +#define QMP_MBOX_IPC_MAX_PARAMS		5
>> +
>> +#define QMP_MAX_PARAM_IN_PARAM_ID	14
>> +#define QMP_PARAM_CNT_FOR_OUTBUF	3
>> +#define QMP_SRAM_IPC_MAX_PARAMS		(QMP_MAX_PARAM_IN_PARAM_ID * QMP_PARAM_CNT_FOR_OUTBUF)
>> +#define QMP_SRAM_IPC_MAX_BUF_SIZE	(QMP_SRAM_IPC_MAX_PARAMS * sizeof(u32))
> 
> These should be expressed in terms of structures and sizeof() instead,
> as well
> 
ok

>> +
>> +#define TMEL_ERROR_GENERIC		(0x1u)
>> +#define TMEL_ERROR_NOT_SUPPORTED	(0x2u)
>> +#define TMEL_ERROR_BAD_PARAMETER	(0x3u)
>> +#define TMEL_ERROR_BAD_MESSAGE		(0x4u)
>> +#define TMEL_ERROR_BAD_ADDRESS		(0x5u)
>> +#define TMEL_ERROR_TMELCOM_FAILURE	(0x6u)
>> +#define TMEL_ERROR_TMEL_BUSY		(0x7u)
> 
> Oh I didn't notice this during the first review.. I assume these are
> returned by the mbox. Please create a dictionary such as:
> 
> u32 tmel_error_dict[] = {
> 	[TMEL_ERROR_GENERIC] = EINVAL,
> 	[TMEL_ERROR_NOT_SUPPORTED] = EOPNOTSUPP
> 	...
> };
> 
> that we can then plug into the function down below that currently does
> error ? -EINVAL : 0
> 

Hmm ok, will try this mapping.

Regards,
  Sricharan

Re: [PATCH V4 2/2] mailbox: tmelite-qmp: Introduce TMEL QMP mailbox driver

[Sricharan Ramabadhran]

On 4/26/2025 3:19 PM, Konrad Dybcio wrote:
> On 3/27/25 7:17 PM, Sricharan R wrote:
>> From: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>>
>> This mailbox facilitates the communication between the TMEL server
>> subsystem (Trust Management Engine Lite) and the TMEL client
>> (APPSS/BTSS/AUDIOSS), used for secure services like secure image
>> authentication, enable/disable efuses, crypto services etc. Each client in
>> the SoC has its own block of message RAM and IRQ for communication with the
>> TMEL SS. The protocol used to communicate in the message RAM is known as
>> Qualcomm Messaging Protocol (QMP).
>>
>> Remote proc driver subscribes to this mailbox and uses the
>> mbox_send_message to use TMEL to securely authenticate/teardown the images.
>>
>> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>> ---
> 
> [...]
> 
>> +
>> +#define QMP_NUM_CHANS		0x1
> 
> Quantities make more sense in decimal, but since this is effectively
> a single-use value, you can put in the '1' literal in num_chans and use
> devm_kzalloc instead of devm_kcalloc in the other use
> 
ok

>> +#define QMP_TOUT_MS		1000
> 
> "TIMEOUT"
>
ok

>> +#define QMP_CTRL_DATA_SIZE	4
>> +#define QMP_MAX_PKT_SIZE	0x18
> 
> This is very handwavy, please structurize all data that comes in and
> out of the mailbox.
>
ok

>> +#define QMP_UCORE_DESC_OFFSET	0x1000
>> +#define QMP_SEND_TIMEOUT	30000
> 
> Please include the unit in the macro name - although 30s is quite a
> timeout for a couple bytes..
> 
ok, will reduce timeout as well

> [...]
> 
>> +#define QMP_HW_MBOX_SIZE		32
>> +#define QMP_MBOX_RSV_SIZE		4
>> +#define QMP_MBOX_IPC_PACKET_SIZE	(QMP_HW_MBOX_SIZE - QMP_CTRL_DATA_SIZE - QMP_MBOX_RSV_SIZE)
>> +#define QMP_MBOX_IPC_MAX_PARAMS		5
>> +
>> +#define QMP_MAX_PARAM_IN_PARAM_ID	14
>> +#define QMP_PARAM_CNT_FOR_OUTBUF	3
>> +#define QMP_SRAM_IPC_MAX_PARAMS		(QMP_MAX_PARAM_IN_PARAM_ID * QMP_PARAM_CNT_FOR_OUTBUF)
>> +#define QMP_SRAM_IPC_MAX_BUF_SIZE	(QMP_SRAM_IPC_MAX_PARAMS * sizeof(u32))
> 
> These should be expressed in terms of structures and sizeof() instead,
> as well
ok

> 
>> +
>> +#define TMEL_ERROR_GENERIC		(0x1u)
>> +#define TMEL_ERROR_NOT_SUPPORTED	(0x2u)
>> +#define TMEL_ERROR_BAD_PARAMETER	(0x3u)
>> +#define TMEL_ERROR_BAD_MESSAGE		(0x4u)
>> +#define TMEL_ERROR_BAD_ADDRESS		(0x5u)
>> +#define TMEL_ERROR_TMELCOM_FAILURE	(0x6u)
>> +#define TMEL_ERROR_TMEL_BUSY		(0x7u)
> 
> Oh I didn't notice this during the first review.. I assume these are
> returned by the mbox. Please create a dictionary such as:
> 
> u32 tmel_error_dict[] = {
> 	[TMEL_ERROR_GENERIC] = EINVAL,
> 	[TMEL_ERROR_NOT_SUPPORTED] = EOPNOTSUPP
> 	...
> };
> 
> that we can then plug into the function down below that currently does
> error ? -EINVAL : 0

ok, agree. will add.

Regards,
Sricharan

Re: [PATCH V4 1/2] dt-bindings: mailbox: Document qcom,ipq5424-tmel

[Bjorn Andersson]

On Tue, Apr 15, 2025 at 04:38:44PM +0530, Sricharan Ramabadhran wrote:
> 
> 
> On 4/1/2025 12:59 PM, Sricharan Ramabadhran wrote:
> > 
> > 
> > On 3/28/2025 1:32 PM, Krzysztof Kozlowski wrote:
> > > On Thu, Mar 27, 2025 at 11:47:49PM +0530, Sricharan R wrote:
> > > > +properties:
> > > > +  compatible:
> > > > +    items:
> > > > +      - enum:
> > > > +          - qcom,ipq5424-tmel
> > > 
> > > blank line
> > ok
> > 
> > > 
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  mboxes:
> > > > +    maxItems: 1
> > > 
> > > Why mbox is having an mbox? This does not look right and suggest the
> > > block is misrepresented. I read the diagram and description two times
> > > and still do not see how this fits there.
> > TMEL/QMP secure functionalities are exposed to clients (like rproc) by
> > registering TMEL as mailbox controller. The IPC bit to communicate with
> > the TMEL/QMP controller itself is handled by the apcs mailbox. Hence
> > it looks like a mbox inside mbox.
> > 
> > Alternatively, would it be fine to fold the IPC bit handling in this
> > driver itself for doing the regmap_write (instead of connecting to
> > apcs mailbox separately) ?
> > 

The APCS provides the interface for triggering interrupts on the remote
processors, and mailbox with NULL messages is how we expose this to the
clients. On some platforms this mechanism is exposed in the form of IPCC
instead, i.e. another mailbox provider.

It might not be a mailbox, but it's the closest thing we had and it's
what we use everywhere else.

> > Also, there are couple of other ways of designing this as well.
> > Earlier i mentioned this in the RFC post [1] for getting the design
> > sorted out.
> > 
> > [1] https://lore.kernel.org/lkml/20241205080633.2623142-1-
> > quic_srichara@quicinc.com/T/
> > 
> > -------------------------------------------------------------
> > Had the below mentioned in the RFC earlier.
> > 
> > The intention of posting this is to get the design reviewed/corrected
> > since there are also other possible ways of having this SS support like:
> > 
> > a) Make TMEL QMP as a 'rpmsg' driver and clients can connect using
> >     rmpsg_send
> > 
> > b) Keep TMEL APIs seperately in drivers/firmware which would export APIs
> >     and QMP mailbox seperately.
> >     Clients can then call the exported APIS.
> > 
> > c) Combine both TMEL and QMP as mailbox (this is the approach used here)
> > 
> 
> Hi Krysztof,
> 
> Can you kindly provide your suggestion on how to proceed for the above ?
> Would want to align on the design approach before posting the next
> version.
> 

How does the TME QMP interface differ from the QMP implementation in
drivers/qcom/qcom_aoss.c?

In the AOSS QMP case we determined that there was no benefit to
abstracting this interface through the mailbox API - and a _hardware_
mailbox doesn't take variable length strings as input...


The concept of posting variable length messages onto a communication
channel resembles rpmsg, but at least the AOSS QMP is a single-channel
mechanism and there's no mapping to "rpmsg clients" in this model.

Will the TMEL QMP interface be used by anything but the TMEL driver? Why
should the TMEL and QMP drivers be separated?

Why is the mailbox API the proper abstraction of the TMEL interface?

Regards,
Bjorn

Re: [PATCH V4 1/2] dt-bindings: mailbox: Document qcom,ipq5424-tmel

[Kathiravan Thirumoorthy]

On 12/18/2025 9:56 AM, Bjorn Andersson wrote:

Thanks Bjorn for the comments. I will be taking over this series since 
Sricharan is moved to other team. I will review and get back to your 
comments post new year vacation. Thanks.

> On Tue, Apr 15, 2025 at 04:38:44PM +0530, Sricharan Ramabadhran wrote:
>>
>> On 4/1/2025 12:59 PM, Sricharan Ramabadhran wrote:
>>>
>>> On 3/28/2025 1:32 PM, Krzysztof Kozlowski wrote:
>>>> On Thu, Mar 27, 2025 at 11:47:49PM +0530, Sricharan R wrote:
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    items:
>>>>> +      - enum:
>>>>> +          - qcom,ipq5424-tmel
>>>> blank line
>>> ok
>>>
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  interrupts:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  mboxes:
>>>>> +    maxItems: 1
>>>> Why mbox is having an mbox? This does not look right and suggest the
>>>> block is misrepresented. I read the diagram and description two times
>>>> and still do not see how this fits there.
>>> TMEL/QMP secure functionalities are exposed to clients (like rproc) by
>>> registering TMEL as mailbox controller. The IPC bit to communicate with
>>> the TMEL/QMP controller itself is handled by the apcs mailbox. Hence
>>> it looks like a mbox inside mbox.
>>>
>>> Alternatively, would it be fine to fold the IPC bit handling in this
>>> driver itself for doing the regmap_write (instead of connecting to
>>> apcs mailbox separately) ?
>>>
> The APCS provides the interface for triggering interrupts on the remote
> processors, and mailbox with NULL messages is how we expose this to the
> clients. On some platforms this mechanism is exposed in the form of IPCC
> instead, i.e. another mailbox provider.
>
> It might not be a mailbox, but it's the closest thing we had and it's
> what we use everywhere else.
>
>>> Also, there are couple of other ways of designing this as well.
>>> Earlier i mentioned this in the RFC post [1] for getting the design
>>> sorted out.
>>>
>>> [1] https://lore.kernel.org/lkml/20241205080633.2623142-1-
>>> quic_srichara@quicinc.com/T/
>>>
>>> -------------------------------------------------------------
>>> Had the below mentioned in the RFC earlier.
>>>
>>> The intention of posting this is to get the design reviewed/corrected
>>> since there are also other possible ways of having this SS support like:
>>>
>>> a) Make TMEL QMP as a 'rpmsg' driver and clients can connect using
>>>      rmpsg_send
>>>
>>> b) Keep TMEL APIs seperately in drivers/firmware which would export APIs
>>>      and QMP mailbox seperately.
>>>      Clients can then call the exported APIS.
>>>
>>> c) Combine both TMEL and QMP as mailbox (this is the approach used here)
>>>
>> Hi Krysztof,
>>
>> Can you kindly provide your suggestion on how to proceed for the above ?
>> Would want to align on the design approach before posting the next
>> version.
>>
> How does the TME QMP interface differ from the QMP implementation in
> drivers/qcom/qcom_aoss.c?
>
> In the AOSS QMP case we determined that there was no benefit to
> abstracting this interface through the mailbox API - and a _hardware_
> mailbox doesn't take variable length strings as input...
>
>
> The concept of posting variable length messages onto a communication
> channel resembles rpmsg, but at least the AOSS QMP is a single-channel
> mechanism and there's no mapping to "rpmsg clients" in this model.
>
> Will the TMEL QMP interface be used by anything but the TMEL driver? Why
> should the TMEL and QMP drivers be separated?
>
> Why is the mailbox API the proper abstraction of the TMEL interface?
>
> Regards,
> Bjorn
>