git: 35f93203963f - main - scmi: Introduce a new SCMI API and port CLK SCMI driver to it

From: Andrew Turner <andrew_at_FreeBSD.org>
Date: Thu, 11 Apr 2024 09:59:29 UTC
The branch main has been updated by andrew:

URL: https://cgit.FreeBSD.org/src/commit/?id=35f93203963f83161012cd731e858a56548c2ef9

commit 35f93203963f83161012cd731e858a56548c2ef9
Author:     Cristian Marussi <cristian.marussi@arm.com>
AuthorDate: 2023-12-11 08:33:01 +0000
Commit:     Andrew Turner <andrew@FreeBSD.org>
CommitDate: 2024-04-11 09:58:56 +0000

    scmi: Introduce a new SCMI API and port CLK SCMI driver to it
    
    Expose new scmi_buf_get/put API methods to build and send messages;
    command request descriptors are now pre-allocated when the SCMI core is
    initialized and kept in a free list, instead of being allocated on the
    stack of the caller of the SCMI request.
    
    Dynamically allocated descriptors enable the SCMI core to keep around
    and track outstanding transactions for as long as needed, outliving the
    lifetime of the caller stack: this allows tracking of late or missing
    replies and it will be needed when adding support for SCMI transports
    that allows for more messages to be inflight concurrently.
    
    Move the existing CLK SCMI driver to the new API.
    
    Reviewed by:    andrew
    Tested on:      Arm Morello Board
    Sponsored by:   Arm Ltd
    Differential Revision:  https://reviews.freebsd.org/D43046
---
 sys/dev/firmware/arm/scmi.c         | 282 +++++++++++++++++++++++++++++++++---
 sys/dev/firmware/arm/scmi.h         |  32 ++--
 sys/dev/firmware/arm/scmi_clk.c     | 232 +++++++++++++----------------
 sys/dev/firmware/arm/scmi_clk.h     |  13 +-
 sys/dev/firmware/arm/scmi_if.m      |   8 +-
 sys/dev/firmware/arm/scmi_mailbox.c |  36 ++---
 sys/dev/firmware/arm/scmi_shmem.c   |  39 ++---
 sys/dev/firmware/arm/scmi_shmem.h   |  10 +-
 sys/dev/firmware/arm/scmi_smc.c     |  30 ++--
 9 files changed, 433 insertions(+), 249 deletions(-)

diff --git a/sys/dev/firmware/arm/scmi.c b/sys/dev/firmware/arm/scmi.c
index 620b40ba32aa..945c2b2e9f6e 100644
--- a/sys/dev/firmware/arm/scmi.c
+++ b/sys/dev/firmware/arm/scmi.c
@@ -42,6 +42,7 @@
 #include <sys/module.h>
 #include <sys/mutex.h>
 #include <sys/queue.h>
+#include <sys/refcount.h>
 
 #include <dev/clk/clk.h>
 #include <dev/fdt/simplebus.h>
@@ -87,6 +88,31 @@
 #define SCMI_MSG_TOKEN(_hdr)		\
     (((_hdr) & SCMI_HDR_TOKEN_M) >> SCMI_HDR_TOKEN_S)
 
+struct scmi_req {
+	int		cnt;
+	bool		timed_out;
+	bool		use_polling;
+	bool		done;
+	struct mtx	mtx;
+	LIST_ENTRY(scmi_req)	next;
+	int		protocol_id;
+	int		message_id;
+	int		token;
+	uint32_t	header;
+	struct scmi_msg msg;
+};
+
+#define buf_to_msg(b)	__containerof((b), struct scmi_msg, payld)
+#define msg_to_req(m)	__containerof((m), struct scmi_req, msg)
+#define buf_to_req(b)	msg_to_req(buf_to_msg(b))
+
+LIST_HEAD(reqs_head, scmi_req);
+
+struct scmi_reqs_pool {
+	struct mtx		mtx;
+	struct reqs_head	head;
+};
+
 BITSET_DEFINE(_scmi_tokens, SCMI_MAX_TOKEN);
 LIST_HEAD(inflight_head, scmi_req);
 #define	REQHASH(_sc, _tk)		\
@@ -97,11 +123,19 @@ struct scmi_transport {
 	struct _scmi_tokens	avail_tokens;
 	struct inflight_head	*inflight_ht;
 	unsigned long		inflight_mask;
+	struct scmi_reqs_pool	*chans[SCMI_CHAN_MAX];
 	struct mtx		mtx;
 };
 
 static int		scmi_transport_init(struct scmi_softc *);
 static void		scmi_transport_cleanup(struct scmi_softc *);
+static struct scmi_reqs_pool *scmi_reqs_pool_allocate(const int, const int);
+static void		scmi_reqs_pool_free(struct scmi_reqs_pool *);
+static struct scmi_req *scmi_req_alloc(struct scmi_softc *, enum scmi_chan);
+static void		scmi_req_free_unlocked(struct scmi_softc *,
+    enum scmi_chan, struct scmi_req *);
+static void		scmi_req_get(struct scmi_softc *, struct scmi_req *);
+static void		scmi_req_put(struct scmi_softc *, struct scmi_req *);
 static int		scmi_token_pick(struct scmi_softc *);
 static void		scmi_token_release_unlocked(struct scmi_softc *, int);
 static int		scmi_req_track_inflight(struct scmi_softc *,
@@ -111,7 +145,7 @@ static int		scmi_req_drop_inflight(struct scmi_softc *,
 static struct scmi_req *scmi_req_lookup_inflight(struct scmi_softc *, uint32_t);
 
 static int		scmi_wait_for_response(struct scmi_softc *,
-    struct scmi_req *);
+			    struct scmi_req *, void **);
 static void		scmi_process_response(struct scmi_softc *, uint32_t);
 
 int
@@ -177,6 +211,42 @@ DEFINE_CLASS_1(scmi, scmi_driver, scmi_methods, sizeof(struct scmi_softc),
 DRIVER_MODULE(scmi, simplebus, scmi_driver, 0, 0);
 MODULE_VERSION(scmi, 1);
 
+static struct scmi_reqs_pool *
+scmi_reqs_pool_allocate(const int max_msg, const int max_payld_sz)
+{
+	struct scmi_reqs_pool *rp;
+	struct scmi_req *req;
+
+	rp = malloc(sizeof(*rp), M_DEVBUF, M_ZERO | M_WAITOK);
+
+	LIST_INIT(&rp->head);
+	for (int i = 0; i < max_msg; i++) {
+		req = malloc(sizeof(*req) + max_payld_sz,
+		    M_DEVBUF, M_ZERO | M_WAITOK);
+
+		mtx_init(&req->mtx, "req", "SCMI", MTX_SPIN);
+		LIST_INSERT_HEAD(&rp->head, req, next);
+	}
+
+	mtx_init(&rp->mtx, "reqs_pool", "SCMI", MTX_SPIN);
+
+	return (rp);
+}
+
+static void
+scmi_reqs_pool_free(struct scmi_reqs_pool *rp)
+{
+	struct scmi_req *req;
+
+	LIST_FOREACH(req, &rp->head, next) {
+		mtx_destroy(&req->mtx);
+		free(req, M_DEVBUF);
+	}
+
+	mtx_destroy(&rp->mtx);
+	free(rp, M_DEVBUF);
+}
+
 static int
 scmi_transport_init(struct scmi_softc *sc)
 {
@@ -191,9 +261,26 @@ scmi_transport_init(struct scmi_softc *sc)
 	trs->inflight_ht = hashinit(SCMI_MAX_MSG, M_DEVBUF,
 	    &trs->inflight_mask);
 
+	trs->chans[SCMI_CHAN_A2P] =
+	    scmi_reqs_pool_allocate(SCMI_MAX_MSG, SCMI_MAX_MSG_PAYLD_SIZE);
+	if (trs->chans[SCMI_CHAN_A2P] == NULL) {
+		free(trs, M_DEVBUF);
+		return (ENOMEM);
+	}
+
+	trs->chans[SCMI_CHAN_P2A] =
+	    scmi_reqs_pool_allocate(SCMI_MAX_MSG, SCMI_MAX_MSG_PAYLD_SIZE);
+	if (trs->chans[SCMI_CHAN_P2A] == NULL) {
+		scmi_reqs_pool_free(trs->chans[SCMI_CHAN_A2P]);
+		free(trs, M_DEVBUF);
+		return (ENOMEM);
+	}
+
 	sc->trs = trs;
 	ret = SCMI_TRANSPORT_INIT(sc->dev);
 	if (ret != 0) {
+		scmi_reqs_pool_free(trs->chans[SCMI_CHAN_A2P]);
+		scmi_reqs_pool_free(trs->chans[SCMI_CHAN_P2A]);
 		free(trs, M_DEVBUF);
 		return (ret);
 	}
@@ -207,9 +294,72 @@ scmi_transport_cleanup(struct scmi_softc *sc)
 	SCMI_TRANSPORT_CLEANUP(sc->dev);
 	mtx_destroy(&sc->trs->mtx);
 	hashdestroy(sc->trs->inflight_ht, M_DEVBUF, sc->trs->inflight_mask);
+	scmi_reqs_pool_free(sc->trs->chans[SCMI_CHAN_A2P]);
+	scmi_reqs_pool_free(sc->trs->chans[SCMI_CHAN_P2A]);
 	free(sc->trs, M_DEVBUF);
 }
 
+static struct scmi_req *
+scmi_req_alloc(struct scmi_softc *sc, enum scmi_chan ch_idx)
+{
+	struct scmi_reqs_pool *rp;
+	struct scmi_req *req = NULL;
+
+	rp = sc->trs->chans[ch_idx];
+	mtx_lock_spin(&rp->mtx);
+	if (!LIST_EMPTY(&rp->head)) {
+		req = LIST_FIRST(&rp->head);
+		LIST_REMOVE_HEAD(&rp->head, next);
+	}
+	mtx_unlock_spin(&rp->mtx);
+
+	if (req != NULL)
+		refcount_init(&req->cnt, 1);
+
+	return (req);
+}
+
+static void
+scmi_req_free_unlocked(struct scmi_softc *sc, enum scmi_chan ch_idx,
+    struct scmi_req *req)
+{
+	struct scmi_reqs_pool *rp;
+
+	rp = sc->trs->chans[ch_idx];
+	mtx_lock_spin(&rp->mtx);
+	req->timed_out = false;
+	req->done = false;
+	refcount_init(&req->cnt, 0);
+	LIST_INSERT_HEAD(&rp->head, req, next);
+	mtx_unlock_spin(&rp->mtx);
+}
+
+static void
+scmi_req_get(struct scmi_softc *sc, struct scmi_req *req)
+{
+	bool ok;
+
+	mtx_lock_spin(&req->mtx);
+	ok = refcount_acquire_if_not_zero(&req->cnt);
+	mtx_unlock_spin(&req->mtx);
+
+	if (!ok)
+		device_printf(sc->dev, "%s() -- BAD REFCOUNT\n", __func__);
+
+	return;
+}
+
+static void
+scmi_req_put(struct scmi_softc *sc, struct scmi_req *req)
+{
+	mtx_lock_spin(&req->mtx);
+	if (!refcount_release_if_not_last(&req->cnt)) {
+		bzero(&req->msg, sizeof(req->msg) + SCMI_MAX_MSG_PAYLD_SIZE);
+		scmi_req_free_unlocked(sc, SCMI_CHAN_A2P, req);
+	}
+	mtx_unlock_spin(&req->mtx);
+}
+
 static int
 scmi_token_pick(struct scmi_softc *sc)
 {
@@ -260,7 +410,8 @@ scmi_finalize_req(struct scmi_softc *sc, struct scmi_req *req)
 	header |= req->protocol_id << SCMI_HDR_PROTOCOL_ID_S;
 	header |= req->token << SCMI_HDR_TOKEN_S;
 
-	req->msg_header = htole32(header);
+	req->header = htole32(header);
+	req->msg.hdr = htole32(header);
 
 	return (0);
 }
@@ -275,7 +426,9 @@ scmi_req_track_inflight(struct scmi_softc *sc, struct scmi_req *req)
 	if (error != 0)
 		return (error);
 
-	/* TODO Review/simplify locking around inflight ?*/
+	/* Bump refcount to get hold of this in-flight transaction */
+	scmi_req_get(sc, req);
+	/* Register in the inflight hashtable */
 	mtx_lock_spin(&sc->trs->mtx);
 	LIST_INSERT_HEAD(REQHASH(sc, req->token), req, next);
 	mtx_unlock_spin(&sc->trs->mtx);
@@ -287,10 +440,13 @@ static int
 scmi_req_drop_inflight(struct scmi_softc *sc, struct scmi_req *req)
 {
 
+	/* Remove from inflight hashtable at first ... */
 	mtx_lock_spin(&sc->trs->mtx);
 	LIST_REMOVE(req, next);
 	scmi_token_release_unlocked(sc, req->token);
 	mtx_unlock_spin(&sc->trs->mtx);
+	/* ...and drop refcount..potentially releasing *req */
+	scmi_req_put(sc, req);
 
 	return (0);
 }
@@ -315,6 +471,7 @@ scmi_req_lookup_inflight(struct scmi_softc *sc, uint32_t hdr)
 static void
 scmi_process_response(struct scmi_softc *sc, uint32_t hdr)
 {
+	bool timed_out = false;
 	struct scmi_req *req;
 
 	req = scmi_req_lookup_inflight(sc, hdr);
@@ -325,8 +482,24 @@ scmi_process_response(struct scmi_softc *sc, uint32_t hdr)
 		return;
 	}
 
+	mtx_lock_spin(&req->mtx);
 	req->done = true;
-	wakeup(req);
+	if (!req->timed_out)
+		wakeup(req);
+	else
+		timed_out = true;
+	mtx_unlock_spin(&req->mtx);
+
+	if (timed_out)
+		device_printf(sc->dev,
+		    "Late reply for timed-out request - token: 0x%X. Ignore.\n",
+		    req->token);
+
+	/*
+	 * In case of a late reply to a timed-out transaction this will
+	 * finally free the pending scmi_req
+	 */
+	scmi_req_drop_inflight(sc, req);
 }
 
 void
@@ -346,51 +519,124 @@ scmi_rx_irq_callback(device_t dev, void *chan, uint32_t hdr)
 }
 
 static int
-scmi_wait_for_response(struct scmi_softc *sc, struct scmi_req *req)
+scmi_wait_for_response(struct scmi_softc *sc, struct scmi_req *req, void **out)
 {
 	int ret;
 
-	if (req->use_polling) {
-		ret = SCMI_POLL_MSG(sc->dev, req, sc->trs_desc.reply_timo_ms);
+	if (req->msg.polling) {
+		bool needs_drop;
+
+		ret = SCMI_POLL_MSG(sc->dev, &req->msg,
+		    sc->trs_desc.reply_timo_ms);
+		/*
+		 * Drop reference to successfully polled req unless it had
+		 * already also been processed on the IRQ path.
+		 * Addresses a possible race-condition between polling and
+		 * interrupt reception paths.
+		 */
+		mtx_lock_spin(&req->mtx);
+		needs_drop = (ret == 0) && !req->done;
+		mtx_unlock_spin(&req->mtx);
+		if (needs_drop)
+			scmi_req_drop_inflight(sc, req);
+		if (ret == 0 && req->msg.hdr != req->header) {
+			device_printf(sc->dev,
+			    "Malformed reply with header |%08X|. Expected: |%08X|Drop.\n",
+			    le32toh(req->msg.hdr), le32toh(req->header));
+		}
 	} else {
 		ret = tsleep(req, 0, "scmi_wait4",
 		    (sc->trs_desc.reply_timo_ms * hz) / 1000);
 		/* Check for lost wakeups since there is no associated lock */
+		mtx_lock_spin(&req->mtx);
 		if (ret != 0 && req->done)
 			ret = 0;
+		mtx_unlock_spin(&req->mtx);
 	}
 
-	if (ret == 0)
-		SCMI_COLLECT_REPLY(sc->dev, req);
-	else
+	if (ret == 0) {
+		SCMI_COLLECT_REPLY(sc->dev, &req->msg);
+		if (req->msg.payld[0] != 0)
+			ret = req->msg.payld[0];
+		*out = &req->msg.payld[SCMI_MSG_HDR_SIZE];
+	} else {
+		mtx_lock_spin(&req->mtx);
+		req->timed_out = true;
+		mtx_unlock_spin(&req->mtx);
 		device_printf(sc->dev,
 		    "Request for token 0x%X timed-out.\n", req->token);
+	}
 
 	SCMI_TX_COMPLETE(sc->dev, NULL);
 
 	return (ret);
 }
 
+void *
+scmi_buf_get(device_t dev, uint8_t protocol_id, uint8_t message_id,
+    int tx_payld_sz, int rx_payld_sz)
+{
+	struct scmi_softc *sc;
+	struct scmi_req *req;
+
+	sc = device_get_softc(dev);
+
+	if (tx_payld_sz > SCMI_MAX_MSG_PAYLD_SIZE ||
+	    rx_payld_sz > SCMI_MAX_MSG_REPLY_SIZE) {
+		device_printf(dev, "Unsupported payload size. Drop.\n");
+		return (NULL);
+	}
+
+	/* Pick one from free list */
+	req = scmi_req_alloc(sc, SCMI_CHAN_A2P);
+	if (req == NULL)
+		return (NULL);
+
+	req->protocol_id = protocol_id & SCMI_HDR_PROTOCOL_ID_BF;
+	req->message_id = message_id & SCMI_HDR_MESSAGE_ID_BF;
+	req->msg.tx_len = sizeof(req->msg.hdr) + tx_payld_sz;
+	req->msg.rx_len = rx_payld_sz ?
+	    rx_payld_sz + 2 * sizeof(uint32_t) : SCMI_MAX_MSG_SIZE;
+
+	return (&req->msg.payld[0]);
+}
+
+void
+scmi_buf_put(device_t dev, void *buf)
+{
+	struct scmi_softc *sc;
+	struct scmi_req *req;
+
+	sc = device_get_softc(dev);
+
+	req = buf_to_req(buf);
+	scmi_req_put(sc, req);
+}
+
 int
-scmi_request(device_t dev, struct scmi_req *req)
+scmi_request(device_t dev, void *in, void **out)
 {
 	struct scmi_softc *sc;
+	struct scmi_req *req;
 	int error;
 
 	sc = device_get_softc(dev);
 
-	req->use_polling = cold || sc->trs_desc.no_completion_irq;
+	req = buf_to_req(in);
+
+	req->msg.polling =
+	    (cold || sc->trs_desc.no_completion_irq || req->use_polling);
 
 	/* Set inflight and send using transport specific method - refc-2 */
 	error = scmi_req_track_inflight(sc, req);
 	if (error != 0)
 		return (error);
 
-	error = SCMI_XFER_MSG(sc->dev, req);
-	if (error == 0)
-		error = scmi_wait_for_response(sc, req);
-
-	scmi_req_drop_inflight(sc, req);
+	error = SCMI_XFER_MSG(sc->dev, &req->msg);
+	if (error != 0) {
+		scmi_req_drop_inflight(sc, req);
+		return (error);
+	}
 
-	return (error);
+	return (scmi_wait_for_response(sc, req, out));
 }
diff --git a/sys/dev/firmware/arm/scmi.h b/sys/dev/firmware/arm/scmi.h
index 361e56c76212..572422594292 100644
--- a/sys/dev/firmware/arm/scmi.h
+++ b/sys/dev/firmware/arm/scmi.h
@@ -34,11 +34,10 @@
 
 #include "scmi_if.h"
 
-
-#define dprintf(fmt, ...)
-
 #define SCMI_MAX_MSG		32
-#define SCMI_MSG_HDR_SIZE	(sizeof(uint32_t))
+#define SCMI_MAX_MSG_PAYLD_SIZE	128
+#define SCMI_MAX_MSG_REPLY_SIZE	(SCMI_MAX_MSG_PAYLD_SIZE - sizeof(uint32_t))
+#define SCMI_MAX_MSG_SIZE	(SCMI_MAX_MSG_PAYLD_SIZE + sizeof(uint32_t))
 
 enum scmi_chan {
 	SCMI_CHAN_A2P,
@@ -61,26 +60,23 @@ struct scmi_softc {
 	struct scmi_transport		*trs;
 };
 
-struct scmi_req {
-	bool		use_polling;
-	bool		done;
-	LIST_ENTRY(scmi_req)	next;
-	int		protocol_id;
-	int		message_id;
-	int		token;
-	uint32_t	msg_header;
-	const void	*in_buf;
-	uint32_t	in_size;
-	void		*out_buf;
-	uint32_t	out_size;
+struct scmi_msg {
+	bool		polling;
+	uint32_t	tx_len;
+	uint32_t	rx_len;
+#define SCMI_MSG_HDR_SIZE	(sizeof(uint32_t))
+	uint32_t	hdr;
+	uint8_t		payld[];
 };
 
-int scmi_request(device_t dev, struct scmi_req *req);
+void *scmi_buf_get(device_t dev, uint8_t protocol_id, uint8_t message_id,
+		   int tx_payd_sz, int rx_payld_sz);
+void scmi_buf_put(device_t dev, void *buf);
+int scmi_request(device_t dev, void *in, void **);
 void scmi_rx_irq_callback(device_t dev, void *chan, uint32_t hdr);
 
 DECLARE_CLASS(scmi_driver);
 
 int scmi_attach(device_t dev);
-int scmi_request(device_t dev, struct scmi_req *req);
 
 #endif /* !_ARM64_SCMI_SCMI_H_ */
diff --git a/sys/dev/firmware/arm/scmi_clk.c b/sys/dev/firmware/arm/scmi_clk.c
index b9ab2145ed76..d5cfb335008b 100644
--- a/sys/dev/firmware/arm/scmi_clk.c
+++ b/sys/dev/firmware/arm/scmi_clk.c
@@ -2,6 +2,7 @@
  * SPDX-License-Identifier: BSD-2-Clause
  *
  * Copyright (c) 2022 Ruslan Bukin <br@bsdpad.com>
+ * Copyright (c) 2023 Arm Ltd
  *
  * This work was supported by Innovate UK project 105694, "Digital Security
  * by Design (DSbD) Technology Platform Prototype".
@@ -58,88 +59,68 @@ struct scmi_clknode_softc {
 static int
 scmi_clk_get_rate(struct scmi_clk_softc *sc, int clk_id, uint64_t *rate)
 {
-	struct scmi_clk_rate_get_out out;
-	struct scmi_clk_rate_get_in in;
-	struct scmi_req req;
+	struct scmi_clk_rate_get_out *out;
+	struct scmi_clk_rate_get_in *in;
 	int error;
 
-	req.protocol_id = SCMI_PROTOCOL_ID_CLOCK;
-	req.message_id = SCMI_CLOCK_RATE_GET;
-	req.in_buf = &in;
-	req.in_size = sizeof(struct scmi_clk_rate_get_in);
-	req.out_buf = &out;
-	req.out_size = sizeof(struct scmi_clk_rate_get_out);
-
-	in.clock_id = clk_id;
-
-	error = scmi_request(sc->scmi, &req);
-	if (error != 0)
-		return (error);
-
-	if (out.status != 0)
+	in = scmi_buf_get(sc->scmi, SCMI_PROTOCOL_ID_CLOCK,
+	    SCMI_CLOCK_RATE_GET, sizeof(*in), sizeof(*out));
+	if (in == NULL)
 		return (ENXIO);
 
-	*rate = out.rate_lsb | ((uint64_t)out.rate_msb << 32);
+	in->clock_id = clk_id;
+	error = scmi_request(sc->scmi, in, (void **)&out);
+	if (error == 0)
+		*rate = out->rate_lsb | ((uint64_t)out->rate_msb << 32);
 
-	return (0);
+	scmi_buf_put(sc->scmi, in);
+
+	return (error);
 }
 
 static int
 scmi_clk_set_rate(struct scmi_clk_softc *sc, int clk_id, uint64_t rate)
 {
-	struct scmi_clk_rate_set_out out;
-	struct scmi_clk_rate_set_in in;
-	struct scmi_req req;
+	struct scmi_clk_rate_set_in *in;
+	void *out;
 	int error;
 
-	req.protocol_id = SCMI_PROTOCOL_ID_CLOCK;
-	req.message_id = SCMI_CLOCK_RATE_SET;
-	req.in_buf = &in;
-	req.in_size = sizeof(struct scmi_clk_rate_set_in);
-	req.out_buf = &out;
-	req.out_size = sizeof(struct scmi_clk_rate_set_out);
+	in = scmi_buf_get(sc->scmi, SCMI_PROTOCOL_ID_CLOCK,
+	    SCMI_CLOCK_RATE_SET, sizeof(*in), 0);
+	if (in == NULL)
+		return (ENXIO);
 
-	in.clock_id = clk_id;
-	in.flags = SCMI_CLK_RATE_ROUND_CLOSEST;
-	in.rate_lsb = (uint32_t)rate;
-	in.rate_msb = (uint32_t)(rate >> 32);
+	in->clock_id = clk_id;
+	in->flags = SCMI_CLK_RATE_ROUND_CLOSEST;
+	in->rate_lsb = (uint32_t)rate;
+	in->rate_msb = (uint32_t)(rate >> 32);
 
-	error = scmi_request(sc->scmi, &req);
-	if (error != 0)
-		return (error);
+	error = scmi_request(sc->scmi, in, &out);
 
-	if (out.status != 0)
-		return (ENXIO);
+	scmi_buf_put(sc->scmi, in);
 
-	return (0);
+	return (error);
 }
 
 static int __unused
 scmi_clk_gate(struct scmi_clk_softc *sc, int clk_id, int enable)
 {
-	struct scmi_clk_state_out out;
-	struct scmi_clk_state_in in;
-	struct scmi_req req;
+	struct scmi_clk_state_in *in;
+	void *out;
 	int error;
 
-	req.protocol_id = SCMI_PROTOCOL_ID_CLOCK;
-	req.message_id = SCMI_CLOCK_CONFIG_SET;
-	req.in_buf = &in;
-	req.in_size = sizeof(struct scmi_clk_state_in);
-	req.out_buf = &out;
-	req.out_size = sizeof(struct scmi_clk_state_out);
-
-	in.clock_id = clk_id;
-	in.attributes = enable;
+	in = scmi_buf_get(sc->scmi, SCMI_PROTOCOL_ID_CLOCK,
+	    SCMI_CLOCK_CONFIG_SET, sizeof(*in), 0);
+	if (in == NULL)
+		return (ENXIO);
 
-	error = scmi_request(sc->scmi, &req);
-	if (error != 0)
-		return (error);
+	in->clock_id = clk_id;
+	in->attributes = enable;
+	error = scmi_request(sc->scmi, in, &out);
 
-	if (out.status != 0)
-		return (ENXIO);
+	scmi_buf_put(sc->scmi, in);
 
-	return (0);
+	return (error);
 }
 
 static int
@@ -178,8 +159,6 @@ scmi_clknode_set_freq(struct clknode *clk, uint64_t fin, uint64_t *fout,
 	clk_sc = clknode_get_softc(clk);
 	sc = device_get_softc(clk_sc->dev);
 
-	dprintf("%s: %ld\n", __func__, *fout);
-
 	scmi_clk_set_rate(sc, clk_sc->clock_id, *fout);
 
 	*stop = 1;
@@ -235,71 +214,60 @@ scmi_clk_add_node(struct scmi_clk_softc *sc, int index, char *clock_name)
 static int
 scmi_clk_get_name(struct scmi_clk_softc *sc, int index, char **result)
 {
-	struct scmi_clk_name_get_out out;
-	struct scmi_clk_name_get_in in;
-	struct scmi_req req;
-	char *clock_name;
+	struct scmi_clk_name_get_out *out;
+	struct scmi_clk_name_get_in *in;
 	int error;
 
-	req.protocol_id = SCMI_PROTOCOL_ID_CLOCK;
-	req.message_id = SCMI_CLOCK_NAME_GET;
-	req.in_buf = &in;
-	req.in_size = sizeof(struct scmi_clk_name_get_in);
-	req.out_buf = &out;
-	req.out_size = sizeof(struct scmi_clk_name_get_out);
-
-	in.clock_id = index;
-
-	error = scmi_request(sc->scmi, &req);
-	if (error != 0)
-		return (error);
-
-	if (out.status != 0)
+	in = scmi_buf_get(sc->scmi, SCMI_PROTOCOL_ID_CLOCK,
+	    SCMI_CLOCK_NAME_GET, sizeof(*in), sizeof(*out));
+	if (in == NULL)
 		return (ENXIO);
 
-	clock_name = malloc(sizeof(out.name), M_DEVBUF, M_WAITOK);
-	strncpy(clock_name, out.name, sizeof(out.name));
+	in->clock_id = index;
+	error = scmi_request(sc->scmi, in, (void **)&out);
+	if (error == 0) {
+		char *clock_name;
 
-	*result = clock_name;
+		clock_name = malloc(sizeof(out->name), M_DEVBUF, M_WAITOK);
+		strncpy(clock_name, out->name, sizeof(out->name));
+		*result = clock_name;
+	}
 
-	return (0);
+	scmi_buf_put(sc->scmi, in);
+
+	return (error);
 }
 
 static int
 scmi_clk_attrs(struct scmi_clk_softc *sc, int index)
 {
-	struct scmi_clk_attrs_out out;
-	struct scmi_clk_attrs_in in;
-	struct scmi_req req;
-	int error;
+	struct scmi_clk_attrs_out *out;
+	struct scmi_clk_attrs_in *in;
 	char *clock_name;
+	int error;
 
-	req.protocol_id = SCMI_PROTOCOL_ID_CLOCK;
-	req.message_id = SCMI_CLOCK_ATTRIBUTES;
-	req.in_buf = &in;
-	req.in_size = sizeof(struct scmi_clk_attrs_in);
-	req.out_buf = &out;
-	req.out_size = sizeof(struct scmi_clk_attrs_out);
-
-	in.clock_id = index;
-
-	error = scmi_request(sc->scmi, &req);
-	if (error != 0)
-		return (error);
-
-	if (out.status != 0)
+	in = scmi_buf_get(sc->scmi, SCMI_PROTOCOL_ID_CLOCK,
+	    SCMI_CLOCK_ATTRIBUTES, sizeof(*in), sizeof(*out));
+	if (in == NULL)
 		return (ENXIO);
 
-	if (out.attributes & CLK_ATTRS_EXT_CLK_NAME) {
-		error = scmi_clk_get_name(sc, index, &clock_name);
-		if (error)
-			return (error);
-	} else {
-		clock_name = malloc(sizeof(out.clock_name), M_DEVBUF, M_WAITOK);
-		strncpy(clock_name, out.clock_name, sizeof(out.clock_name));
+	in->clock_id = index;
+	error = scmi_request(sc->scmi, in, (void **)&out);
+	if (error == 0) {
+		if (out->attributes & CLK_ATTRS_EXT_CLK_NAME) {
+			error = scmi_clk_get_name(sc, index, &clock_name);
+		} else {
+			clock_name = malloc(sizeof(out->clock_name),
+			    M_DEVBUF, M_WAITOK);
+			strncpy(clock_name, out->clock_name,
+			    sizeof(out->clock_name));
+		}
+
+		if (error == 0)
+			error = scmi_clk_add_node(sc, index, clock_name);
 	}
 
-	error = scmi_clk_add_node(sc, index, clock_name);
+	scmi_buf_put(sc->scmi, in);
 
 	return (error);
 }
@@ -307,47 +275,45 @@ scmi_clk_attrs(struct scmi_clk_softc *sc, int index)
 static int
 scmi_clk_discover(struct scmi_clk_softc *sc)
 {
-	struct scmi_clk_protocol_attrs_out out;
-	struct scmi_req req;
+	struct scmi_clk_protocol_attrs_out *out;
+	void *in;
 	int nclocks;
 	int failing;
 	int error;
 	int i;
 
-	req.protocol_id = SCMI_PROTOCOL_ID_CLOCK;
-	req.message_id = SCMI_PROTOCOL_ATTRIBUTES;
-	req.in_buf = NULL;
-	req.in_size = 0;
-	req.out_buf = &out;
-	req.out_size = sizeof(struct scmi_clk_protocol_attrs_out);
-
-	error = scmi_request(sc->scmi, &req);
-	if (error != 0)
-		return (error);
-
-	if (out.status != 0)
+	in = scmi_buf_get(sc->scmi, SCMI_PROTOCOL_ID_CLOCK,
+	    SCMI_PROTOCOL_ATTRIBUTES, 0, sizeof(*out));
+	if (in == NULL)
 		return (ENXIO);
 
-	nclocks = (out.attributes & CLK_ATTRS_NCLOCKS_M) >>
-	    CLK_ATTRS_NCLOCKS_S;
+	error = scmi_request(sc->scmi, in, (void **)&out);
+	if (error == 0) {
+		nclocks = (out->attributes & CLK_ATTRS_NCLOCKS_M) >>
+		    CLK_ATTRS_NCLOCKS_S;
 
-	device_printf(sc->dev, "Found %d clocks.\n", nclocks);
+		device_printf(sc->dev, "Found %d clocks.\n", nclocks);
 
-	failing = 0;
+		failing = 0;
 
-	for (i = 0; i < nclocks; i++) {
-		error = scmi_clk_attrs(sc, i);
-		if (error) {
-			device_printf(sc->dev,
-			    "Could not process clock index %d.\n", i);
-			failing++;
+		for (i = 0; i < nclocks; i++) {
+			error = scmi_clk_attrs(sc, i);
+			if (error) {
+				device_printf(sc->dev,
+				    "Could not process clock index %d.\n", i);
+				failing++;
+				error = 0;
+			}
 		}
+		if (failing == nclocks)
+			error = ENXIO;
+	} else {
+		error = ENXIO;
 	}
 
-	if (failing == nclocks)
-		return (ENXIO);
+	scmi_buf_put(sc->scmi, in);
 
-	return (0);
+	return (error);
 }
 
 static int
diff --git a/sys/dev/firmware/arm/scmi_clk.h b/sys/dev/firmware/arm/scmi_clk.h
index a293b00d846e..d987586a6e8e 100644
--- a/sys/dev/firmware/arm/scmi_clk.h
+++ b/sys/dev/firmware/arm/scmi_clk.h
@@ -2,6 +2,7 @@
  * SPDX-License-Identifier: BSD-2-Clause
  *
  * Copyright (c) 2022 Ruslan Bukin <br@bsdpad.com>
+ * Copyright (c) 2023 Arm Ltd
  *
  * This work was supported by Innovate UK project 105694, "Digital Security
  * by Design (DSbD) Technology Platform Prototype".
@@ -36,7 +37,6 @@
  */
 
 struct scmi_clk_protocol_attrs_out {
-	int32_t status;
 	uint32_t attributes;
 #define	CLK_ATTRS_NCLOCKS_S		0
 #define	CLK_ATTRS_NCLOCKS_M		(0xffff << CLK_ATTRS_NCLOCKS_S)
@@ -47,7 +47,6 @@ struct scmi_clk_attrs_in {
 };
 
 struct scmi_clk_attrs_out {
-	int32_t status;
 	uint32_t attributes;
 #define	CLK_ATTRS_RATE_CHANGE_NOTIFY_SUPP	(1 << 31)
 #define	CLK_ATTRS_RATE_REQ_CHANGE_NOTIFY_SUPP	(1 << 30)
@@ -62,7 +61,6 @@ struct scmi_clk_name_get_in {
 };
 
 struct scmi_clk_name_get_out {
-	int32_t status;
 	uint32_t flags;
 	uint8_t name[64];
 };
@@ -86,16 +84,11 @@ struct scmi_clk_state_in {
 	uint32_t attributes;
 };
 
-struct scmi_clk_state_out {
-	int32_t status;
-};
-
 struct scmi_clk_rate_get_in {
 	uint32_t clock_id;
 };
 
 struct scmi_clk_rate_get_out {
-	int32_t status;
 	uint32_t rate_lsb;
 	uint32_t rate_msb;
 };
@@ -107,8 +100,4 @@ struct scmi_clk_rate_set_in {
 	uint32_t rate_msb;
 };
 
-struct scmi_clk_rate_set_out {
-	int32_t status;
-};
-
 #endif /* !_ARM64_SCMI_SCMI_CLK_H_ */
diff --git a/sys/dev/firmware/arm/scmi_if.m b/sys/dev/firmware/arm/scmi_if.m
index ab9adb911fda..a8d606406e50 100644
--- a/sys/dev/firmware/arm/scmi_if.m
+++ b/sys/dev/firmware/arm/scmi_if.m
@@ -28,7 +28,7 @@
 INTERFACE scmi;
 
 HEADER {
-	struct scmi_req;
+	struct scmi_msg;
 };
 
 METHOD int transport_init {
@@ -41,18 +41,18 @@ METHOD void transport_cleanup {
 
 METHOD int xfer_msg {
 	device_t dev;
-	struct scmi_req *req;
+	struct scmi_msg *msg;
 };
 
 METHOD int poll_msg {
 	device_t dev;
-	struct scmi_req *req;
+	struct scmi_msg *msg;
 	unsigned int tmo;
 };
 
 METHOD int collect_reply {
 	device_t dev;
-	struct scmi_req *req;
+	struct scmi_msg *msg;
 };
 
 METHOD void tx_complete {
diff --git a/sys/dev/firmware/arm/scmi_mailbox.c b/sys/dev/firmware/arm/scmi_mailbox.c
index 5d53294f4378..858b81f68845 100644
--- a/sys/dev/firmware/arm/scmi_mailbox.c
+++ b/sys/dev/firmware/arm/scmi_mailbox.c
@@ -36,10 +36,8 @@
 #include <sys/bus.h>
 #include <sys/cpu.h>
 #include <sys/kernel.h>
-#include <sys/lock.h>
 #include <sys/module.h>
 
-#include <dev/clk/clk.h>
 #include <dev/fdt/simplebus.h>
 #include <dev/fdt/fdt_common.h>
 #include <dev/ofw/ofw_bus_subr.h>
@@ -60,10 +58,10 @@ struct scmi_mailbox_softc {
 
 static int	scmi_mailbox_transport_init(device_t);
 static void	scmi_mailbox_transport_cleanup(device_t);
-static int	scmi_mailbox_xfer_msg(device_t, struct scmi_req *);
-static int	scmi_mailbox_poll_msg(device_t, struct scmi_req *,
+static int	scmi_mailbox_xfer_msg(device_t, struct scmi_msg *);
+static int	scmi_mailbox_poll_msg(device_t, struct scmi_msg *,
     unsigned int);
-static int	scmi_mailbox_collect_reply(device_t, struct scmi_req *);
+static int	scmi_mailbox_collect_reply(device_t, struct scmi_msg *);
 static void	scmi_mailbox_tx_complete(device_t, void *);
 
 static int	scmi_mailbox_probe(device_t);
@@ -129,27 +127,26 @@ scmi_mailbox_transport_cleanup(device_t dev)
 }
 
 static int
-scmi_mailbox_xfer_msg(device_t dev, struct scmi_req *req)
+scmi_mailbox_xfer_msg(device_t dev, struct scmi_msg *msg)
 {
 	struct scmi_mailbox_softc *sc;
 	int ret;
 
*** 283 LINES SKIPPED ***