Re: [PATCH v5 2/3] firmware: add Exynos ACPM protocol driver

[Krzysztof Kozlowski]

On 20/12/2024 15:32, Tudor Ambarus wrote:
> diff --git a/drivers/firmware/samsung/Kconfig b/drivers/firmware/samsung/Kconfig
> new file mode 100644
> index 000000000000..750b41342174
> --- /dev/null
> +++ b/drivers/firmware/samsung/Kconfig
> @@ -0,0 +1,14 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config EXYNOS_ACPM_PROTOCOL
> +	tristate "Exynos Alive Clock and Power Manager (ACPM) Message Protocol"
> +	depends on ARCH_EXYNOS || COMPILE_TEST
> +	depends on EXYNOS_MBOX

Is it build time dependency? No || COMPILE_TEST?

Is it fine when EXYNOS_MBOX is a module?

> +	help
> +	  Alive Clock and Power Manager (ACPM) Message Protocol is defined for
> +	  the purpose of communication between the ACPM firmware and masters
> +	  (AP, AOC, ...). ACPM firmware operates on the Active Power Management
> +	  (APM) module that handles overall power activities.
> +
> +	  This protocol driver provides interface for all the client drivers
> +	  making use of the features offered by the APM.
> diff --git a/drivers/firmware/samsung/Makefile b/drivers/firmware/samsung/Makefile
> new file mode 100644
> index 000000000000..7b4c9f6f34f5
> --- /dev/null
> +++ b/drivers/firmware/samsung/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +acpm-protocol-objs			:= exynos-acpm.o exynos-acpm-pmic.o
> +obj-$(CONFIG_EXYNOS_ACPM_PROTOCOL)	+= acpm-protocol.o
> diff --git a/drivers/firmware/samsung/exynos-acpm-pmic.c b/drivers/firmware/samsung/exynos-acpm-pmic.c
> new file mode 100644
> index 000000000000..d698e5a03630
> --- /dev/null
> +++ b/drivers/firmware/samsung/exynos-acpm-pmic.c
> @@ -0,0 +1,224 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2020 Samsung Electronics Co., Ltd.
> + * Copyright 2020 Google LLC.
> + * Copyright 2024 Linaro Ltd.
> + */
> +#include <linux/bitfield.h>
> +#include <linux/firmware/samsung/exynos-acpm-protocol.h>
> +#include <linux/ktime.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +
> +#include "exynos-acpm.h"
> +#include "exynos-acpm-pmic.h"
> +
> +#define ACPM_PMIC_CHANNEL		GENMASK(15, 12)
> +#define ACPM_PMIC_TYPE			GENMASK(11, 8)
> +#define ACPM_PMIC_REG			GENMASK(7, 0)
> +
> +#define ACPM_PMIC_RETURN		GENMASK(31, 24)
> +#define ACPM_PMIC_MASK			GENMASK(23, 16)
> +#define ACPM_PMIC_VALUE			GENMASK(15, 8)
> +#define ACPM_PMIC_FUNC			GENMASK(7, 0)
> +
> +#define ACPM_PMIC_BULK_SHIFT		8
> +#define ACPM_PMIC_BULK_MASK		GENMASK(7, 0)
> +#define ACPM_PMIC_BULK_MAX_COUNT	8
> +
> +enum exynos_acpm_pmic_func {
> +	ACPM_PMIC_READ,
> +	ACPM_PMIC_WRITE,
> +	ACPM_PMIC_UPDATE,
> +	ACPM_PMIC_BULK_READ,
> +	ACPM_PMIC_BULK_WRITE,
> +};
> +
> +static inline u32 acpm_pmic_set_bulk(u32 data, unsigned int i)
> +{
> +	return (data & ACPM_PMIC_BULK_MASK) << (ACPM_PMIC_BULK_SHIFT * i);
> +}
> +
> +static inline u32 acpm_pmic_get_bulk(u32 data, unsigned int i)
> +{
> +	return (data >> (ACPM_PMIC_BULK_SHIFT * i)) & ACPM_PMIC_BULK_MASK;
> +}
> +
> +static void acpm_dvfs_set_xfer(struct acpm_xfer *xfer, u32 *cmd,
> +			       int acpm_chan_id)
> +{
> +	xfer->tx.cmd = cmd;
> +	xfer->tx.len = sizeof(cmd);
> +	xfer->rx.cmd = cmd;
> +	xfer->rx.len = sizeof(cmd);
> +	xfer->acpm_chan_id = acpm_chan_id;
> +}
> +
> +static void acpm_pmic_init_read_cmd(u32 *cmd, u8 type, u8 reg, u8 chan)
> +{
> +	cmd[0] = FIELD_PREP(ACPM_PMIC_TYPE, type) |
> +		 FIELD_PREP(ACPM_PMIC_REG, reg) |
> +		 FIELD_PREP(ACPM_PMIC_CHANNEL, chan);
> +	cmd[1] = FIELD_PREP(ACPM_PMIC_FUNC, ACPM_PMIC_READ);
> +	cmd[3] = ktime_to_ms(ktime_get());
> +}
> +
> +int acpm_pmic_read_reg(const struct acpm_handle *handle, int acpm_chan_id,
> +		       u8 type, u8 reg, u8 chan, u8 *dest)
> +{
> +	struct acpm_xfer xfer;
> +	u32 cmd[4] = {0};
> +	int ret;
> +
> +	acpm_pmic_init_read_cmd(cmd, type, reg, chan);
> +	acpm_dvfs_set_xfer(&xfer, cmd, acpm_chan_id);
> +
> +	ret = acpm_do_xfer(handle, &xfer);
> +	if (ret)
> +		return ret;
> +
> +	*dest = FIELD_GET(ACPM_PMIC_VALUE, xfer.rx.cmd[1]);
> +
> +	return FIELD_GET(ACPM_PMIC_RETURN, xfer.rx.cmd[1]);
> +}
> +
> +static void acpm_pmic_init_bulk_read_cmd(u32 *cmd, u8 type, u8 reg, u8 chan,
> +					 u8 count)
> +{
> +	cmd[0] = FIELD_PREP(ACPM_PMIC_TYPE, type) |
> +		 FIELD_PREP(ACPM_PMIC_REG, reg) |
> +		 FIELD_PREP(ACPM_PMIC_CHANNEL, chan);
> +	cmd[1] = FIELD_PREP(ACPM_PMIC_FUNC, ACPM_PMIC_BULK_READ) |
> +		 FIELD_PREP(ACPM_PMIC_VALUE, count);
> +}
> +
> +int acpm_pmic_bulk_read(const struct acpm_handle *handle, int acpm_chan_id,
> +			u8 type, u8 reg, u8 chan, u8 count, u8 *buf)
> +{
> +	struct acpm_xfer xfer;
> +	u32 cmd[4] = {0};
> +	int i, ret;
> +
> +	if (count > ACPM_PMIC_BULK_MAX_COUNT)
> +		return -EINVAL;
> +
> +	acpm_pmic_init_bulk_read_cmd(cmd, type, reg, chan, count);
> +	acpm_dvfs_set_xfer(&xfer, cmd, acpm_chan_id);
> +
> +	ret = acpm_do_xfer(handle, &xfer);
> +	if (ret)
> +		return ret;
> +
> +	ret = FIELD_GET(ACPM_PMIC_RETURN, xfer.rx.cmd[1]);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < count; i++) {
> +		if (i < 4)
> +			buf[i] = acpm_pmic_get_bulk(xfer.rx.cmd[2], i);
> +		else
> +			buf[i] = acpm_pmic_get_bulk(xfer.rx.cmd[3], i - 4);
> +	}
> +
> +	return 0;
> +}
> +
> +static void acpm_pmic_init_write_cmd(u32 *cmd, u8 type, u8 reg, u8 chan,
> +				     u8 value)
> +{
> +	cmd[0] = FIELD_PREP(ACPM_PMIC_TYPE, type) |
> +		 FIELD_PREP(ACPM_PMIC_REG, reg) |
> +		 FIELD_PREP(ACPM_PMIC_CHANNEL, chan);
> +	cmd[1] = FIELD_PREP(ACPM_PMIC_FUNC, ACPM_PMIC_WRITE) |
> +		 FIELD_PREP(ACPM_PMIC_VALUE, value);
> +	cmd[3] = ktime_to_ms(ktime_get());
> +}
> +
> +int acpm_pmic_write_reg(const struct acpm_handle *handle, int acpm_chan_id,
> +			u8 type, u8 reg, u8 chan, u8 value)
> +{
> +	struct acpm_xfer xfer;
> +	u32 cmd[4] = {0};
> +	int ret;
> +
> +	acpm_pmic_init_write_cmd(cmd, type, reg, chan, value);
> +	acpm_dvfs_set_xfer(&xfer, cmd, acpm_chan_id);
> +
> +	ret = acpm_do_xfer(handle, &xfer);
> +	if (ret)
> +		return ret;
> +
> +	return FIELD_GET(ACPM_PMIC_RETURN, xfer.rx.cmd[1]);
> +}
> +
> +static void acpm_pmic_init_bulk_write_cmd(u32 *cmd, u8 type, u8 reg, u8 chan,
> +					  u8 count, u8 *buf)

u32 cmd[4] - I think newer GCC could use of that. At least reader will know.

const u8 *buf

Same in all other places, where applicable

> +{
> +	int i;
> +
> +	cmd[0] = FIELD_PREP(ACPM_PMIC_TYPE, type) |
> +		 FIELD_PREP(ACPM_PMIC_REG, reg) |
> +		 FIELD_PREP(ACPM_PMIC_CHANNEL, chan);
> +	cmd[1] = FIELD_PREP(ACPM_PMIC_FUNC, ACPM_PMIC_BULK_WRITE) |
> +		 FIELD_PREP(ACPM_PMIC_VALUE, count);
> +
> +	for (i = 0; i < count; i++) {
> +		if (i < 4)
> +			cmd[2] |= acpm_pmic_set_bulk(buf[i], i);
> +		else
> +			cmd[3] |= acpm_pmic_set_bulk(buf[i], i - 4);
> +	}
> +}
> +
> +int acpm_pmic_bulk_write(const struct acpm_handle *handle, int acpm_chan_id,
> +			 u8 type, u8 reg, u8 chan, u8 count, u8 *buf)


const u8 *buf

> +{
> +	struct acpm_xfer xfer;
> +	u32 cmd[4] = {0};
> +	int ret;
> +
> +	if (count > ACPM_PMIC_BULK_MAX_COUNT)
> +		return -EINVAL;
> +
> +	acpm_pmic_init_bulk_write_cmd(cmd, type, reg, chan, count, buf);
> +	acpm_dvfs_set_xfer(&xfer, cmd, acpm_chan_id);
> +
> +	ret = acpm_do_xfer(handle, &xfer);
> +	if (ret)
> +		return ret;
> +
> +	return FIELD_GET(ACPM_PMIC_RETURN, xfer.rx.cmd[1]);
> +}
> +
> +static void acpm_pmic_init_update_cmd(u32 *cmd, u8 type, u8 reg, u8 chan,
> +				      u8 value, u8 mask)
> +{
> +	cmd[0] = FIELD_PREP(ACPM_PMIC_TYPE, type) |
> +		 FIELD_PREP(ACPM_PMIC_REG, reg) |
> +		 FIELD_PREP(ACPM_PMIC_CHANNEL, chan);
> +	cmd[1] = FIELD_PREP(ACPM_PMIC_FUNC, ACPM_PMIC_UPDATE) |
> +		 FIELD_PREP(ACPM_PMIC_VALUE, value) |
> +		 FIELD_PREP(ACPM_PMIC_MASK, mask);
> +	cmd[3] = ktime_to_ms(ktime_get());
> +}
> +
> +int acpm_pmic_update_reg(const struct acpm_handle *handle, int acpm_chan_id,
> +			 u8 type, u8 reg, u8 chan, u8 value, u8 mask)
> +{
> +	struct acpm_xfer xfer;
> +	u32 cmd[4] = {0};
> +	int ret;
> +
> +	acpm_pmic_init_update_cmd(cmd, type, reg, chan, value, mask);
> +	acpm_dvfs_set_xfer(&xfer, cmd, acpm_chan_id);
> +
> +	ret = acpm_do_xfer(handle, &xfer);
> +	if (ret)
> +		return ret;
> +
> +	return FIELD_GET(ACPM_PMIC_RETURN, xfer.rx.cmd[1]);
> +}

I have troubles understanding the split. I would assume PMIC uses the
ACPM to talk with PMIC... or somehow the opposite, but here this is all
mixed. You have acpm_pmic_update_reg() which uses  ACPM code
(acpm_do_xfer()), but ACPM code also references acpm_pmic_update_reg()
and others. This is a circular dependency between objects, which
compiles and works fine but is confusing.

> +
> +MODULE_AUTHOR("Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("Samsung Exynos ACPM mailbox PMIC protocol driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/firmware/samsung/exynos-acpm-pmic.h b/drivers/firmware/samsung/exynos-acpm-pmic.h
> new file mode 100644
> index 000000000000..92b1997d9933
> --- /dev/null
> +++ b/drivers/firmware/samsung/exynos-acpm-pmic.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright 2020 Samsung Electronics Co., Ltd.
> + * Copyright 2020 Google LLC.
> + * Copyright 2024 Linaro Ltd.
> + */
> +#ifndef __EXYNOS_ACPM_PMIC_H__
> +#define __EXYNOS_ACPM_PMIC_H__
> +
> +#include <linux/types.h>
> +
> +struct acpm_handle;
> +
> +int acpm_pmic_read_reg(const struct acpm_handle *handle, int acpm_chan_id,
> +		       u8 type, u8 reg, u8 chan, u8 *dest);
> +int acpm_pmic_bulk_read(const struct acpm_handle *handle, int acpm_chan_id,
> +			u8 type, u8 reg, u8 chan, u8 count, u8 *buf);
> +int acpm_pmic_write_reg(const struct acpm_handle *handle, int acpm_chan_id,
> +			u8 type, u8 reg, u8 chan, u8 value);
> +int acpm_pmic_bulk_write(const struct acpm_handle *handle, int acpm_chan_id,
> +			 u8 type, u8 reg, u8 chan, u8 count, u8 *buf);
> +int acpm_pmic_update_reg(const struct acpm_handle *handle, int acpm_chan_id,
> +			 u8 type, u8 reg, u8 chan, u8 value, u8 mask);
> +#endif /* __EXYNOS_ACPM_PMIC_H__ */
> diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
> new file mode 100644
> index 000000000000..2f98306f8325
> --- /dev/null
> +++ b/drivers/firmware/samsung/exynos-acpm.c
> @@ -0,0 +1,805 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2020 Samsung Electronics Co., Ltd.
> + * Copyright 2020 Google LLC.
> + * Copyright 2024 Linaro Ltd.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>
> +#include <linux/bits.h>
> +#include <linux/container_of.h>
> +#include <linux/delay.h>
> +#include <linux/firmware/samsung/exynos-acpm-protocol.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/math.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>

This looks unused

> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include "exynos-acpm.h"
> +#include "exynos-acpm-pmic.h"
> +
> +#define ACPM_PROTOCOL_SEQNUM		GENMASK(21, 16)
> +
> +/* The unit of counter is 20 us. 5000 * 20 = 100 ms */
> +#define ACPM_POLL_TIMEOUT		5000
> +#define ACPM_TX_TIMEOUT_US		500000
> +
> +#define ACPM_GS101_INITDATA_BASE	0xa000
> +
> +/**
> + * struct acpm_shmem - shared memory configuration information.
> + * @reserved:	unused fields.
> + * @chans:	offset to array of struct acpm_chan_shmem.
> + * @reserved1:	unused fields.
> + * @num_chans:	number of channels.
> + */
> +struct acpm_shmem {
> +	u32 reserved[2];
> +	u32 chans;
> +	u32 reserved1[3];
> +	u32 num_chans;
> +};
> +
> +/**
> + * struct acpm_chan_shmem - descriptor of a shared memory channel.
> + *
> + * @id:			channel ID.
> + * @reserved:		unused fields.
> + * @rx_rear:		rear pointer of APM RX queue (TX for AP).
> + * @rx_front:		front pointer of APM RX queue (TX for AP).
> + * @rx_base:		base address of APM RX queue (TX for AP).
> + * @reserved1:		unused fields.
> + * @tx_rear:		rear pointer of APM TX queue (RX for AP).
> + * @tx_front:		front pointer of APM TX queue (RX for AP).
> + * @tx_base:		base address of APM TX queue (RX for AP).
> + * @qlen:		queue length. Applies to both TX/RX queues.
> + * @mlen:		message length. Applies to both TX/RX queues.
> + * @reserved2:		unused fields.
> + * @poll_completion:	true when the channel works on polling.
> + */
> +struct acpm_chan_shmem {
> +	u32 id;
> +	u32 reserved[3];
> +	u32 rx_rear;
> +	u32 rx_front;
> +	u32 rx_base;
> +	u32 reserved1[3];
> +	u32 tx_rear;
> +	u32 tx_front;
> +	u32 tx_base;
> +	u32 qlen;
> +	u32 mlen;
> +	u32 reserved2[2];
> +	u32 poll_completion;
> +};
> +
> +/**
> + * struct acpm_queue - exynos acpm queue.
> + *
> + * @rear:	rear address of the queue.
> + * @front:	front address of the queue.
> + * @base:	base address of the queue.
> + */
> +struct acpm_queue {
> +	void __iomem *rear;
> +	void __iomem *front;
> +	void __iomem *base;
> +};
> +
> +/**
> + * struct acpm_rx_data - RX queue data.
> + *
> + * @cmd:	pointer to where the data shall be saved.
> + * @n_cmd:	number of 32-bit commands.
> + * @response:	true if the client expects the RX data.
> + */
> +struct acpm_rx_data {
> +	u32 *cmd;
> +	size_t n_cmd;
> +	bool response;
> +};
> +
> +#define ACPM_SEQNUM_MAX    64
> +
> +/**
> + * struct acpm_chan - driver internal representation of a channel.
> + * @cl:		mailbox client.
> + * @chan:	mailbox channel.
> + * @acpm:	pointer to driver private data.
> + * @tx:		TX queue. The enqueue is done by the host.
> + *			- front index is written by the host.
> + *			- rear index is written by the firmware.
> + *
> + * @rx:		RX queue. The enqueue is done by the firmware.
> + *			- front index is written by the firmware.
> + *			- rear index is written by the host.
> + * @tx_lock:	protects TX queue.
> + * @rx_lock:	protects RX queue.
> + * @qlen:	queue length. Applies to both TX/RX queues.
> + * @mlen:	message length. Applies to both TX/RX queues.
> + * @seqnum:	sequence number of the last message enqueued on TX queue.
> + * @id:		channel ID.
> + * @poll_completion:	indicates if the transfer needs to be polled for
> + *			completion or interrupt mode is used.
> + * @bitmap_seqnum: bitmap that tracks the messages on the TX/RX queues.
> + * @rx_data:	internal buffer used to drain the RX queue.
> + */
> +struct acpm_chan {
> +	struct mbox_client cl;
> +	struct mbox_chan *chan;
> +	struct acpm_info *acpm;
> +	struct acpm_queue tx;
> +	struct acpm_queue rx;
> +	struct mutex tx_lock;
> +	struct mutex rx_lock;
> +
> +	unsigned int qlen;
> +	unsigned int mlen;
> +	u8 seqnum;
> +	u8 id;
> +	bool poll_completion;
> +
> +	DECLARE_BITMAP(bitmap_seqnum, ACPM_SEQNUM_MAX - 1);
> +	struct acpm_rx_data rx_data[ACPM_SEQNUM_MAX];
> +};
> +
> +/**
> + * struct acpm_info - driver's private data.
> + * @shmem:	pointer to the SRAM configuration data.
> + * @sram_base:	base address of SRAM.
> + * @chans:	pointer to the ACPM channel parameters retrieved from SRAM.
> + * @dev:	pointer to the exynos-acpm device.
> + * @handle:	instance of acpm_handle to send to clients.
> + * @node:	list head
> + * @num_chans:	number of channels available for this controller.
> + * @users:	number of users of this instance.
> + */
> +struct acpm_info {
> +	struct acpm_shmem __iomem *shmem;
> +	void __iomem *sram_base;
> +	struct acpm_chan *chans;
> +	struct device *dev;
> +	struct acpm_handle handle;
> +	struct list_head node;
> +	u32 num_chans;
> +	/* protected by acpm_list_mutex */
> +	int users;
> +};
> +
> +/**
> + * struct acpm_match_data - of_device_id data.
> + * @initdata_base:	offset in SRAM where the channels configuration resides.
> + */
> +struct acpm_match_data {
> +	loff_t initdata_base;
> +};
> +
> +#define client_to_acpm_chan(c) container_of(c, struct acpm_chan, cl)
> +#define handle_to_acpm_info(h) container_of(h, struct acpm_info, handle)
> +
> +/* List of all ACPM devices active in system */
> +static LIST_HEAD(acpm_list);
> +/* Protection for the entire list */
> +static DEFINE_MUTEX(acpm_list_mutex);
> +
> +static inline void acpm_memcpy_fromio32(void *to, const void __iomem *from,
> +					size_t count)
> +{
> +	WARN_ON(!IS_ALIGNED((unsigned long)from, 4) ||
> +		!IS_ALIGNED((unsigned long)to, 4) ||
> +		count % 4);

These should be build time checks if this is necessary.

> +
> +	__ioread32_copy(to, from, count / 4);
> +}
> +
> +static inline void acpm_memcpy_toio32(void __iomem *to, const void *from,
> +				      size_t count)
> +{
> +	WARN_ON(!IS_ALIGNED((unsigned long)to, 4) ||
> +		!IS_ALIGNED((unsigned long)from, 4) ||
> +		count % 4);
> +
> +	__iowrite32_copy(to, from, count / 4);
> +}


...

> +
> +/**
> + * acpm_prepare_xfer() - prepare a transfer before writing the message to the
> + * TX queue.
> + * @achan:	ACPM channel info.
> + * @xfer:	reference to the transfer being prepared.
> + */
> +static void acpm_prepare_xfer(struct acpm_chan *achan, struct acpm_xfer *xfer)
> +{
> +	struct acpm_msg *tx = &xfer->tx;
> +	struct acpm_rx_data *rx_data;
> +
> +	/* Prevent chan->seqnum from being re-used */
> +	do {
> +		if (++achan->seqnum == ACPM_SEQNUM_MAX)
> +			achan->seqnum = 1;
> +	} while (test_bit(achan->seqnum - 1, achan->bitmap_seqnum));
> +
> +	tx->cmd[0] |= FIELD_PREP(ACPM_PROTOCOL_SEQNUM, achan->seqnum);
> +
> +	/* Clear data for upcoming responses */
> +	rx_data = &achan->rx_data[achan->seqnum - 1];
> +	memset(rx_data->cmd, 0, sizeof(*rx_data->cmd) * rx_data->n_cmd);
> +	if (xfer->rx.cmd)
> +		rx_data->response = true;
> +
> +	/* Flag the index based on seqnum. (seqnum: 1~63, bitmap: 0~62) */
> +	set_bit(achan->seqnum - 1, achan->bitmap_seqnum);
> +}
> +
> +/**
> + * acpm_wait_for_message_response - an helper to group all possible ways of
> + * waiting for a synchronous message response.
> + *
> + * @achan:	ACPM channel info.
> + * @xfer:	reference to the transfer being waited for.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int acpm_wait_for_message_response(struct acpm_chan *achan,
> +					  struct acpm_xfer *xfer)
> +{
> +	/* Just polling mode supported for now. */
> +	return acpm_dequeue_by_polling(achan, xfer);
> +}
> +
> +/**
> + * acpm_do_xfer() - do one transfer.
> + * @handle:	pointer to the acpm handle.
> + * @xfer:	transfer to initiate and wait for response.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +int acpm_do_xfer(const struct acpm_handle *handle, struct acpm_xfer *xfer)

const xfer, so it is clear that caller keeps ownership and must ensure
it is valid memory through entire xfer time.

> +{
> +	struct acpm_info *acpm = handle_to_acpm_info(handle);
> +	struct acpm_chan *achan = &acpm->chans[xfer->acpm_chan_id];
> +	struct acpm_msg *tx = &xfer->tx;

Looks like const, same for function argument xfer

> +	u32 idx, tx_front;
> +	int ret;
> +
> +	if (!tx->cmd || tx->len > achan->mlen || xfer->rx.len > achan->mlen)
> +		return -EINVAL;
> +
> +	if (!achan->poll_completion) {
> +		dev_err(achan->acpm->dev, "Interrupt mode not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	mutex_lock(&achan->tx_lock);
> +
> +	tx_front = readl(achan->tx.front);
> +	idx = (tx_front + 1) % achan->qlen;
> +
> +	ret = acpm_wait_for_queue_slots(achan, idx);
> +	if (ret) {
> +		mutex_unlock(&achan->tx_lock);
> +		return ret;
> +	}
> +
> +	acpm_prepare_xfer(achan, xfer);
> +
> +	/* Write TX command. */
> +	acpm_memcpy_toio32(achan->tx.base + achan->mlen * tx_front, tx->cmd,
> +			   tx->len);
> +
> +	/* Advance TX front. */
> +	writel(idx, achan->tx.front);
> +
> +	mutex_unlock(&achan->tx_lock);

Just to be sure I understand correctly:
If concurrent transfer happened exactly now, it would use incremented
tx_front, thus it would not overwrite data written here, right?


> +
> +	ret = mbox_send_message(achan->chan, xfer);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = acpm_wait_for_message_response(achan, xfer);
> +
> +	/*
> +	 * NOTE: we might prefer not to need the mailbox ticker to manage the
> +	 * transfer queueing since the protocol layer queues things by itself.
> +	 * Unfortunately, we have to kick the mailbox framework after we have
> +	 * received our message.
> +	 */
> +	mbox_client_txdone(achan->chan, ret);
> +
> +	return ret;
> +}
> +
> +/**
> + * acpm_put_handle() - release the handle acquired by acpm_get_by_phandle.
> + * @handle:	Handle acquired by acpm_get_by_phandle.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int acpm_put_handle(const struct acpm_handle *handle)
> +{
> +	struct acpm_info *acpm;
> +
> +	if (IS_ERR(handle))
> +		return PTR_ERR(handle);
> +	if (!handle)
> +		return -EINVAL;

I think this cannot happen and you should rather choose: either NULL is
allowed or PTR_ERR, but not both. Although in few cases both ERR and
NULL are desired, so write a comment why here.

> +
> +	acpm = handle_to_acpm_info(handle);
> +	mutex_lock(&acpm_list_mutex);
> +	if (!WARN_ON(!acpm->users))
> +		acpm->users--;

Use refcnt

> +	mutex_unlock(&acpm_list_mutex);
> +
> +	return 0;
> +}
> +
> +/**
> + * devm_acpm_release() - devres release method.
> + * @dev: pointer to device.
> + * @res: pointer to resource.
> + */
> +static void devm_acpm_release(struct device *dev, void *res)
> +{
> +	const struct acpm_handle **ptr = res;
> +	const struct acpm_handle *handle = *ptr;
> +	int ret;
> +
> +	ret = acpm_put_handle(handle);
> +	if (ret)
> +		dev_err(dev, "failed to put handle %d\n", ret);
> +}
> +
> +/**
> + * acpm_get_by_phandle() - get the ACPM handle using DT phandle.
> + * @np:		device node.
> + * @property:	property name containing phandle on ACPM node.
> + *
> + * Return: pointer to handle on success, ERR_PTR(-errno) otherwise.
> + */
> +static const struct acpm_handle *acpm_get_by_phandle(struct device_node *np,
> +						     const char *property)
> +{
> +	struct acpm_handle *handle = NULL;
> +	struct device_node *acpm_np;
> +	struct acpm_info *info;
> +
> +	if (!np) {
> +		pr_err("I need a device pointer\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	acpm_np = of_parse_phandle(np, property, 0);
> +	if (!acpm_np)
> +		return ERR_PTR(-ENODEV);
> +
> +	mutex_lock(&acpm_list_mutex);
> +	list_for_each_entry(info, &acpm_list, node) {
> +		if (acpm_np == info->dev->of_node) {
> +			handle = &info->handle;
> +			info->users++;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&acpm_list_mutex);
> +	of_node_put(acpm_np);
> +

You also need device links and probably try_module_get. See clk.c
clk_hw_create_clk() or of_qcom_ice_get(). Interestingly, none of them
perform both operations, which I think is necessary.

I think you could also avoid entire list and mutex by using
platform_get_drvdata(), see of_qcom_ice_get().


> +	if (!handle)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	return handle;
> +}
> +
> +/**
> + * devm_acpm_get_by_phandle() - managed get handle using phandle.
> + * @dev:	device pointer requesting ACPM handle.
> + * @property:	property name containing phandle on ACPM node.
> + *
> + * Return: pointer to handle on success, ERR_PTR(-errno) otherwise.
> + */
> +const struct acpm_handle *devm_acpm_get_by_phandle(struct device *dev,
> +						   const char *property)
> +{
> +	const struct acpm_handle *handle;
> +	const struct acpm_handle **ptr;
> +
> +	ptr = devres_alloc(devm_acpm_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +	handle = acpm_get_by_phandle(dev_of_node(dev), property);
> +
> +	if (!IS_ERR(handle)) {
> +		*ptr = handle;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +

...

> +#endif /* __EXYNOS_ACPM_H__ */
> diff --git a/include/linux/firmware/samsung/exynos-acpm-protocol.h b/include/linux/firmware/samsung/exynos-acpm-protocol.h
> new file mode 100644
> index 000000000000..f834af20cef8
> --- /dev/null
> +++ b/include/linux/firmware/samsung/exynos-acpm-protocol.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2020 Samsung Electronics Co., Ltd.
> + * Copyright 2020 Google LLC.
> + * Copyright 2024 Linaro Ltd.
> + */
> +
> +#ifndef __EXYNOS_ACPM_PROTOCOL_H
> +#define __EXYNOS_ACPM_PROTOCOL_H
> +
> +#include <linux/types.h>
> +
> +struct acpm_msg {
> +	u32 *cmd;

const most likely, although then you have different message for tx and
tx. Not sure if there is a benefit of having it as structure.

> +	size_t len;
> +};
> +
> +struct acpm_xfer {
> +	struct acpm_msg tx;
> +	struct acpm_msg rx;
> +	int acpm_chan_id;
> +};
> +
> +struct acpm_handle;
> +
> +struct acpm_pmic_ops {
> +	int (*read_reg)(const struct acpm_handle *handle, int acpm_chan_id,
> +			u8 type, u8 reg, u8 chan, u8 *dest);
> +	int (*bulk_read)(const struct acpm_handle *handle, int acpm_chan_id,
> +			 u8 type, u8 reg, u8 chan, u8 count, u8 *buf);
> +	int (*write_reg)(const struct acpm_handle *handle, int acpm_chan_id,
> +			 u8 type, u8 reg, u8 chan, u8 value);
> +	int (*bulk_write)(const struct acpm_handle *handle, int acpm_chan_id,
> +			  u8 type, u8 reg, u8 chan, u8 count, u8 *buf);
> +	int (*update_reg)(const struct acpm_handle *handle, int acpm_chan_id,
> +			  u8 type, u8 reg, u8 chan, u8 value, u8 mask);
> +};
> +
> +struct acpm_ops {
> +	struct acpm_pmic_ops pmic_ops;
> +};
> +
> +/**
> + * struct acpm_handle - Reference to an initialized protocol instance
> + * @ops:
> + */
> +struct acpm_handle {
> +	struct acpm_ops ops;
> +};

All above should not be in public header but private to the driver. In
public header you expose things available to consumers.... which there
are no? So entire header can be next to the driver.

> +
> +struct device;
> +
> +const struct acpm_handle *devm_acpm_get_by_phandle(struct device *dev,
> +						   const char *property);
> +#endif /* __EXYNOS_ACPM_PROTOCOL_H */
> 


Best regards,
Krzysztof