[PATCH v3 0/3] firmware: add exynos ACPM protocol driver

[PATCH v3 0/3] firmware: add exynos ACPM protocol driver

[Tudor Ambarus]

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 library provides the interface for all the client drivers
making use of the features offered by the APM. Add ACPM protocol support.

I mark this as v3 because it is a continuation of:
https://lore.kernel.org/linux-arm-kernel/20241017163649.3007062-1-tudor.ambarus@linaro.org/

The samsung exynos mailbox controller driver, the transport layer for
ACPM, can be found at:
https://lore.kernel.org/linux-arm-kernel/20241205174137.190545-1-tudor.ambarus@linaro.org/

Changes in v3:
- decouple the mailbox controller driver from the ACPM protocol driver
- address Krzysztof's eview comments 
- add ACPM PMIC protocol helpers

Cheers,
ta

Tudor Ambarus (3):
  dt-bindings: firmware: add samsung,exynos-acpm-ipc bindings
  firmware: add exynos ACPM protocol driver
  MAINTAINERS: add entry for the samsung exynos ACPM mailbox protocol

 .../firmware/samsung,exynos-acpm-ipc.yaml     |  64 ++
 MAINTAINERS                                   |  10 +
 drivers/firmware/Kconfig                      |   1 +
 drivers/firmware/Makefile                     |   1 +
 drivers/firmware/samsung/Kconfig              |  14 +
 drivers/firmware/samsung/Makefile             |   4 +
 drivers/firmware/samsung/exynos-acpm-pmic.c   | 226 +++++
 drivers/firmware/samsung/exynos-acpm-pmic.h   |  24 +
 drivers/firmware/samsung/exynos-acpm.c        | 772 ++++++++++++++++++
 drivers/firmware/samsung/exynos-acpm.h        |  15 +
 .../linux/soc/samsung/exynos-acpm-protocol.h  |  57 ++
 11 files changed, 1188 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/firmware/samsung,exynos-acpm-ipc.yaml
 create mode 100644 drivers/firmware/samsung/Kconfig
 create mode 100644 drivers/firmware/samsung/Makefile
 create mode 100644 drivers/firmware/samsung/exynos-acpm-pmic.c
 create mode 100644 drivers/firmware/samsung/exynos-acpm-pmic.h
 create mode 100644 drivers/firmware/samsung/exynos-acpm.c
 create mode 100644 drivers/firmware/samsung/exynos-acpm.h
 create mode 100644 include/linux/soc/samsung/exynos-acpm-protocol.h

-- 
2.47.0.338.g60cca15819-goog

[PATCH v3 1/3] dt-bindings: firmware: add samsung,exynos-acpm-ipc bindings

[Tudor Ambarus]

Add bindings for the samsung exynos ACPM mailbox protocol.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 .../firmware/samsung,exynos-acpm-ipc.yaml     | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/firmware/samsung,exynos-acpm-ipc.yaml

diff --git a/Documentation/devicetree/bindings/firmware/samsung,exynos-acpm-ipc.yaml b/Documentation/devicetree/bindings/firmware/samsung,exynos-acpm-ipc.yaml
new file mode 100644
index 000000000000..bfc9b4af9ea1
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/samsung,exynos-acpm-ipc.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2024 Linaro Ltd.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/firmware/samsung,exynos-acpm-ipc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung exynos ACPM mailbox protocol
+
+maintainers:
+  - Tudor Ambarus <tudor.ambarus@linaro.org>
+
+description: |
+  ACPM (Alive Clock and Power Manager) is a firmware that operates on the
+  APM (Active Power Management) module that handles overall power management
+  activities. ACPM and masters regard each other as independent hardware
+  component and communicate with each other using mailbox messages and
+  shared memory.
+
+  This binding is intended to define the interface the firmware implementing
+  ACPM provides for OSPM in the device tree.
+
+properties:
+  $nodename:
+    const: acpm_ipc
+
+  compatible:
+    const: google,gs101-acpm-ipc
+
+  mboxes:
+    description:
+      List of phandle and mailbox channel specifiers.
+    maxItems: 1
+
+  shmem:
+    description:
+      List of phandle pointing to the shared memory (SHM) area. The memory
+      contains channels configuration data and the TX/RX ring buffers that
+      are used for passing messages to/from the ACPM firmware.
+    maxItems: 1
+
+  initdata-base:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Base address of the configuration data in the shared memory.
+
+additionalProperties: false
+
+required:
+  - compatible
+  - mboxes
+  - shmem
+  - initdata-base
+
+examples:
+  - |
+    firmware {
+        acpm_ipc: acpm_ipc {
+            compatible = "google,gs101-acpm-ipc";
+            mboxes = <&ap2apm_mailbox 0>;
+            shmem = <&apm_sram>;
+            initdata-base = <0xa000>;
+        };
+    };
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 1/3] dt-bindings: firmware: add samsung,exynos-acpm-ipc bindings

[Krzysztof Kozlowski]

On Thu, Dec 05, 2024 at 05:53:43PM +0000, Tudor Ambarus wrote:
> Add bindings for the samsung exynos ACPM mailbox protocol.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  .../firmware/samsung,exynos-acpm-ipc.yaml     | 64 +++++++++++++++++++
>  1 file changed, 64 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/samsung,exynos-acpm-ipc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/firmware/samsung,exynos-acpm-ipc.yaml b/Documentation/devicetree/bindings/firmware/samsung,exynos-acpm-ipc.yaml
> new file mode 100644
> index 000000000000..bfc9b4af9ea1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/samsung,exynos-acpm-ipc.yaml


Filename based on comaptible, so google,gs101-acpm-ipc.yaml

> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2024 Linaro Ltd.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/firmware/samsung,exynos-acpm-ipc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Samsung exynos ACPM mailbox protocol

s/exynos/Exynos/

> +
> +maintainers:
> +  - Tudor Ambarus <tudor.ambarus@linaro.org>
> +
> +description: |
> +  ACPM (Alive Clock and Power Manager) is a firmware that operates on the
> +  APM (Active Power Management) module that handles overall power management
> +  activities. ACPM and masters regard each other as independent hardware
> +  component and communicate with each other using mailbox messages and
> +  shared memory.
> +
> +  This binding is intended to define the interface the firmware implementing
> +  ACPM provides for OSPM in the device tree.
> +
> +properties:
> +  $nodename:
> +    const: acpm_ipc

Drop, no schemas have anything like this.

> +
> +  compatible:
> +    const: google,gs101-acpm-ipc
> +
> +  mboxes:
> +    description:
> +      List of phandle and mailbox channel specifiers.

Drop description.

> +    maxItems: 1
> +
> +  shmem:
> +    description:
> +      List of phandle pointing to the shared memory (SHM) area. The memory
> +      contains channels configuration data and the TX/RX ring buffers that
> +      are used for passing messages to/from the ACPM firmware.
> +    maxItems: 1
> +
> +  initdata-base:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Base address of the configuration data in the shared memory.

If you really need this, then it should be rather encoded as part of
shemm phandle (phandle-array with describing items, just like syscon
phandles are done).

But OTOH, I don't see why this is needed at all so far - compatible
defines it.

> +
> +additionalProperties: false

This should be placed after required block.

> +
> +required:
> +  - compatible
> +  - mboxes
> +  - shmem
> +  - initdata-base
> +
> +examples:
> +  - |
> +    firmware {

Drop

> +        acpm_ipc: acpm_ipc {

Drop unused label.

Node name: power-management (based on your binding description)
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +            compatible = "google,gs101-acpm-ipc";

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: firmware: add samsung,exynos-acpm-ipc bindings

[Tudor Ambarus]

On 12/9/24 8:03 AM, Krzysztof Kozlowski wrote:
>> +  initdata-base:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      Base address of the configuration data in the shared memory.
> If you really need this, then it should be rather encoded as part of
> shemm phandle (phandle-array with describing items, just like syscon
> phandles are done).
> 
> But OTOH, I don't see why this is needed at all so far - compatible
> defines it.

Indeed, I shall move it to compatible. If firmware ever changes, and
uses a different offset to the configuration data, then I can introduce
a new compatible.

I agree with all the other comments as well, will address them in v4.

Thanks!
ta

[PATCH v3 2/3] firmware: add exynos ACPM protocol driver

[Tudor Ambarus]

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.

ACPM and masters regard each other as independent hardware component and
communicate with each other using mailbox messages and shared memory.

This protocol library provides the interface for all the client drivers
making use of the features offered by the APM. Add ACPM protocol support.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/firmware/Kconfig                      |   1 +
 drivers/firmware/Makefile                     |   1 +
 drivers/firmware/samsung/Kconfig              |  14 +
 drivers/firmware/samsung/Makefile             |   4 +
 drivers/firmware/samsung/exynos-acpm-pmic.c   | 226 +++++
 drivers/firmware/samsung/exynos-acpm-pmic.h   |  24 +
 drivers/firmware/samsung/exynos-acpm.c        | 772 ++++++++++++++++++
 drivers/firmware/samsung/exynos-acpm.h        |  15 +
 .../linux/soc/samsung/exynos-acpm-protocol.h  |  57 ++
 9 files changed, 1114 insertions(+)
 create mode 100644 drivers/firmware/samsung/Kconfig
 create mode 100644 drivers/firmware/samsung/Makefile
 create mode 100644 drivers/firmware/samsung/exynos-acpm-pmic.c
 create mode 100644 drivers/firmware/samsung/exynos-acpm-pmic.h
 create mode 100644 drivers/firmware/samsung/exynos-acpm.c
 create mode 100644 drivers/firmware/samsung/exynos-acpm.h
 create mode 100644 include/linux/soc/samsung/exynos-acpm-protocol.h

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 71d8b26c4103..24edb956831b 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -267,6 +267,7 @@ source "drivers/firmware/meson/Kconfig"
 source "drivers/firmware/microchip/Kconfig"
 source "drivers/firmware/psci/Kconfig"
 source "drivers/firmware/qcom/Kconfig"
+source "drivers/firmware/samsung/Kconfig"
 source "drivers/firmware/smccc/Kconfig"
 source "drivers/firmware/tegra/Kconfig"
 source "drivers/firmware/xilinx/Kconfig"
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 7a8d486e718f..91efcc868a05 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -33,6 +33,7 @@ obj-y				+= efi/
 obj-y				+= imx/
 obj-y				+= psci/
 obj-y				+= qcom/
+obj-y				+= samsung/
 obj-y				+= smccc/
 obj-y				+= tegra/
 obj-y				+= xilinx/
diff --git a/drivers/firmware/samsung/Kconfig b/drivers/firmware/samsung/Kconfig
new file mode 100644
index 000000000000..eed8cd8a677b
--- /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
+	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 library 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..371fd67cd56d
--- /dev/null
+++ b/drivers/firmware/samsung/exynos-acpm-pmic.c
@@ -0,0 +1,226 @@
+// 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/module.h>
+#include <linux/sched/clock.h>
+#include <linux/soc/samsung/exynos-acpm-protocol.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_BULK_SHIFT			8
+#define ACPM_BULK_MASK			GENMASK(7, 0)
+#define exynos_acpm_set_bulk(data, i)					\
+	(((data) & ACPM_BULK_MASK) << (ACPM_BULK_SHIFT * (i)))
+#define exynos_acpm_read_bulk(data, i)					\
+	(((data) >> (ACPM_BULK_SHIFT * (i))) & ACPM_BULK_MASK)
+
+enum exynos_acpm_pmic_func {
+	ACPM_PMIC_READ,
+	ACPM_PMIC_WRITE,
+	ACPM_PMIC_UPDATE,
+	ACPM_PMIC_BULK_READ,
+	ACPM_PMIC_BULK_WRITE,
+};
+
+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] = (u32)(sched_clock() / 1000000); /*record ktime ms*/
+}
+
+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);
+
+	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;
+
+	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;
+
+	acpm_pmic_init_bulk_read_cmd(cmd, type, reg, chan, count);
+
+	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;
+
+	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] = exynos_acpm_read_bulk(xfer.rx.cmd[2], i);
+		else
+			buf[i] = exynos_acpm_read_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] = (u32)(sched_clock() / 1000000); /*record ktime ms*/
+}
+
+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);
+
+	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;
+
+	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)
+{
+	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] |= exynos_acpm_set_bulk(buf[i], i);
+		else
+			cmd[3] |= exynos_acpm_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)
+{
+	struct acpm_xfer xfer;
+	u32 cmd[4] = {0};
+	int ret;
+
+	acpm_pmic_init_bulk_write_cmd(cmd, type, reg, chan, count, buf);
+
+	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;
+
+	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] = (u32)(sched_clock() / 1000000); /*record ktime ms*/
+}
+
+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);
+
+	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;
+
+	ret = acpm_do_xfer(handle, &xfer);
+	if (ret)
+		return ret;
+
+	return FIELD_GET(ACPM_PMIC_RETURN, xfer.rx.cmd[1]);
+}
+
+MODULE_AUTHOR("Tudor Ambarus <tudor.ambarus@linaro.org>");
+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..c7ff5fdde25a
--- /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_MFD_H__
+#define __EXYNOS_ACPM_MFD_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_MFD_H__ */
diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
new file mode 100644
index 000000000000..1b04230942a4
--- /dev/null
+++ b/drivers/firmware/samsung/exynos-acpm.c
@@ -0,0 +1,772 @@
+// 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/io.h>
+#include <linux/iopoll.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/math.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/soc/samsung/exynos-acpm-protocol.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
+
+/**
+ * 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;
+	spinlock_t tx_lock;
+	spinlock_t 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;
+};
+
+#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);
+
+/**
+ * acpm_get_rx() - get response from RX queue.
+ * @achan:	ACPM channel info.
+ * @xfer:	reference to the transfer to get response for.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int acpm_get_rx(struct acpm_chan *achan, struct acpm_xfer *xfer)
+{
+	struct acpm_msg *tx = &xfer->tx;
+	struct acpm_msg *rx = &xfer->rx;
+	struct acpm_rx_data *rx_data;
+	const void __iomem *base, *addr;
+	u32 rx_front, rx_seqnum, tx_seqnum, seqnum;
+	u32 i, val, mlen;
+	bool rx_set = false;
+
+	rx_front = readl_relaxed(achan->rx.front);
+	i = readl_relaxed(achan->rx.rear);
+
+	/* Bail out if RX is empty. */
+	if (i == rx_front)
+		return 0;
+
+	base = achan->rx.base;
+	mlen = achan->mlen;
+
+	tx_seqnum = FIELD_GET(ACPM_PROTOCOL_SEQNUM, tx->cmd[0]);
+
+	/* Drain RX queue. */
+	do {
+		/* Read RX seqnum. */
+		addr = base + mlen * i;
+		val = readl_relaxed(addr);
+
+		rx_seqnum = FIELD_GET(ACPM_PROTOCOL_SEQNUM, val);
+		if (!rx_seqnum)
+			return -EIO;
+		/*
+		 * mssg seqnum starts with value 1, whereas the driver considers
+		 * the first mssg at index 0.
+		 */
+		seqnum = rx_seqnum - 1;
+		rx_data = &achan->rx_data[seqnum];
+
+		if (rx_data->response) {
+			if (rx_seqnum == tx_seqnum) {
+				__ioread32_copy(rx->cmd, addr, rx->len / 4);
+				rx_set = true;
+				clear_bit(seqnum, achan->bitmap_seqnum);
+			} else {
+				/*
+				 * The RX data corresponds to another request.
+				 * Save the data to drain the queue, but don't
+				 * clear yet the bitmap. It will be cleared
+				 * after the response is copied to the request.
+				 */
+				__ioread32_copy(rx_data->cmd, addr,
+						rx->len / 4);
+			}
+		} else {
+			clear_bit(seqnum, achan->bitmap_seqnum);
+		}
+
+		i = (i + 1) % achan->qlen;
+	} while (i != rx_front);
+
+	/* We saved all responses, mark RX empty. */
+	writel_relaxed(rx_front, achan->rx.rear);
+
+	/* Flush SRAM posted writes. */
+	readl_relaxed(achan->rx.front);
+
+	/*
+	 * If the response was not in this iteration of the queue, check if the
+	 * RX data was previously saved.
+	 */
+	rx_data = &achan->rx_data[tx_seqnum - 1];
+	if (!rx_set && rx_data->response) {
+		rx_seqnum = FIELD_GET(ACPM_PROTOCOL_SEQNUM,
+				      rx_data->cmd[0]);
+
+		if (rx_seqnum == tx_seqnum) {
+			memcpy(rx->cmd, rx_data->cmd, rx->len);
+			clear_bit(rx_seqnum - 1, achan->bitmap_seqnum);
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * acpm_dequeue_by_polling() - RX dequeue by polling.
+ * @achan:	ACPM channel info.
+ * @xfer:	reference to the transfer being waited for.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int acpm_dequeue_by_polling(struct acpm_chan *achan,
+				   struct acpm_xfer *xfer)
+{
+	bool may_sleep = xfer->flags & ACPM_XFER_MAY_SLEEP;
+	struct device *dev = achan->acpm->dev;
+	struct acpm_msg *tx = &xfer->tx;
+	unsigned long flags;
+	unsigned int cnt_20us = 0;
+	u32 seqnum;
+	int ret;
+
+	seqnum = FIELD_GET(ACPM_PROTOCOL_SEQNUM, tx->cmd[0]);
+
+	do {
+		spin_lock_irqsave(&achan->rx_lock, flags);
+		ret = acpm_get_rx(achan, xfer);
+		spin_unlock_irqrestore(&achan->rx_lock, flags);
+		if (ret)
+			return ret;
+
+		if (!test_bit(seqnum - 1, achan->bitmap_seqnum))
+			return 0;
+
+		/* Determined experimentally. */
+		may_sleep ? usleep_range(20, 30) : udelay(20);
+		cnt_20us++;
+	} while (cnt_20us < ACPM_POLL_TIMEOUT);
+
+	dev_err(dev, "Timeout! ch:%u s:%u bitmap:%lx, cnt_20us = %d.\n",
+		achan->id, seqnum, achan->bitmap_seqnum[0], cnt_20us);
+
+	return -ETIME;
+}
+
+/**
+ * acpm_wait_for_queue_slots() - wait for queue slots.
+ *
+ * @achan:		ACPM channel info.
+ * @next_tx_front:	next front index of the TX queue.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int acpm_wait_for_queue_slots(struct acpm_chan *achan, u32 next_tx_front)
+{
+	u32 val, ret;
+
+	/*
+	 * Wait for RX front to keep up with TX front. Make sure there's at
+	 * least one element between them.
+	 */
+	ret = readl_relaxed_poll_timeout_atomic(achan->rx.front, val,
+						next_tx_front != val, 0,
+						ACPM_TX_TIMEOUT_US);
+	if (ret) {
+		dev_err(achan->acpm->dev, "RX front can not keep up with TX front.\n");
+		return ret;
+	}
+
+	ret = readl_relaxed_poll_timeout_atomic(achan->tx.rear, val,
+						next_tx_front != val, 0,
+						ACPM_TX_TIMEOUT_US);
+	if (ret)
+		dev_err(achan->acpm->dev, "TX queue is full.\n");
+
+	return ret;
+}
+
+/**
+ * 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)
+{
+	if (achan->poll_completion)
+		return acpm_dequeue_by_polling(achan, xfer);
+
+	dev_err(achan->acpm->dev, "Interrupt mode not supported\n");
+	return -EOPNOTSUPP;
+}
+
+/**
+ * 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)
+{
+	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;
+	u32 idx, tx_front;
+	unsigned long flags;
+	int ret;
+
+	if (!tx->cmd || tx->len > achan->mlen || xfer->rx.len > achan->mlen)
+		return -EINVAL;
+
+	spin_lock_irqsave(&achan->tx_lock, flags);
+
+	tx_front = readl_relaxed(achan->tx.front);
+	idx = (tx_front + 1) % achan->qlen;
+
+	ret = acpm_wait_for_queue_slots(achan, idx);
+	if (ret) {
+		spin_unlock_irqrestore(&achan->tx_lock, flags);
+		return ret;
+	}
+
+	acpm_prepare_xfer(achan, xfer);
+
+	/* Write TX command. */
+	__iowrite32_copy(achan->tx.base + achan->mlen * tx_front, tx->cmd,
+			 tx->len / 4);
+
+	/* Advance TX front. */
+	writel_relaxed(idx, achan->tx.front);
+
+	/* Flush SRAM posted writes. */
+	readl_relaxed(achan->tx.front);
+
+	spin_unlock_irqrestore(&achan->tx_lock, flags);
+
+	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;
+
+	acpm = handle_to_acpm_info(handle);
+	mutex_lock(&acpm_list_mutex);
+	if (!WARN_ON(!acpm->users))
+		acpm->users--;
+	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);
+
+	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);
+	}
+
+	return handle;
+}
+EXPORT_SYMBOL_GPL(devm_acpm_get_by_phandle);
+
+/**
+ * acpm_chan_shmem_get_params() - get channel parameters and addresses of the
+ * TX/RX queues.
+ * @achan:	ACPM channel info.
+ * @chan_shmem:	__iomem pointer to a channel described in shared memory.
+ */
+static void acpm_chan_shmem_get_params(struct acpm_chan *achan,
+				struct acpm_chan_shmem __iomem *chan_shmem)
+{
+	void __iomem *base = achan->acpm->sram_base;
+	struct acpm_queue *rx = &achan->rx;
+	struct acpm_queue *tx = &achan->tx;
+
+	achan->mlen = readl_relaxed(&chan_shmem->mlen);
+	achan->poll_completion = readl_relaxed(&chan_shmem->poll_completion);
+	achan->id = readl_relaxed(&chan_shmem->id);
+	achan->qlen = readl_relaxed(&chan_shmem->qlen);
+
+	tx->base = base + readl_relaxed(&chan_shmem->rx_base);
+	tx->rear = base + readl_relaxed(&chan_shmem->rx_rear);
+	tx->front = base + readl_relaxed(&chan_shmem->rx_front);
+
+	rx->base = base + readl_relaxed(&chan_shmem->tx_base);
+	rx->rear = base + readl_relaxed(&chan_shmem->tx_rear);
+	rx->front = base + readl_relaxed(&chan_shmem->tx_front);
+
+	dev_vdbg(achan->acpm->dev, "ID = %d poll = %d, mlen = %d, qlen = %d\n",
+		 achan->id, achan->poll_completion, achan->mlen, achan->qlen);
+}
+
+/**
+ * acpm_achan_alloc_cmds() - allocate buffers for retrieving data from the ACPM
+ * firmware.
+ * @achan:	ACPM channel info.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int acpm_achan_alloc_cmds(struct acpm_chan *achan)
+{
+	struct device *dev = achan->acpm->dev;
+	struct acpm_rx_data *rx_data;
+	size_t cmd_size, n_cmd;
+	int i;
+
+	if (achan->mlen == 0)
+		return 0;
+
+	cmd_size = sizeof(*(achan->rx_data[0].cmd));
+	n_cmd = DIV_ROUND_UP_ULL(achan->mlen, cmd_size);
+
+	for (i = 0; i < ACPM_SEQNUM_MAX; i++) {
+		rx_data = &achan->rx_data[i];
+		rx_data->n_cmd = n_cmd;
+		rx_data->cmd = devm_kcalloc(dev, n_cmd, cmd_size, GFP_KERNEL);
+		if (!rx_data->cmd)
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+/**
+ * acpm_free_mbox_chans() - free mailbox channels.
+ * @acpm:	pointer to driver data.
+ */
+static void acpm_free_mbox_chans(struct acpm_info *acpm)
+{
+	int i;
+
+	for (i = 0; i < acpm->num_chans; i++)
+		mbox_free_channel(acpm->chans[i].chan);
+}
+
+/**
+ * acpm_channels_init() - initialize channels based on the configuration data in
+ * the shared memory.
+ * @acpm:	pointer to driver data.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int acpm_channels_init(struct acpm_info *acpm)
+{
+	struct acpm_shmem __iomem *shmem = acpm->shmem;
+	struct acpm_chan_shmem __iomem *chans_shmem;
+	struct device *dev = acpm->dev;
+	int i, ret;
+
+	acpm->num_chans = readl_relaxed(&shmem->num_chans);
+	acpm->chans = devm_kcalloc(dev, acpm->num_chans, sizeof(*acpm->chans),
+				   GFP_KERNEL);
+	if (!acpm->chans)
+		return -ENOMEM;
+
+	chans_shmem = acpm->sram_base + readl_relaxed(&shmem->chans);
+
+	for (i = 0; i < acpm->num_chans; i++) {
+		struct acpm_chan_shmem __iomem *chan_shmem = &chans_shmem[i];
+		struct acpm_chan *achan = &acpm->chans[i];
+		struct mbox_client *cl = &achan->cl;
+
+		achan->acpm = acpm;
+
+		acpm_chan_shmem_get_params(achan, chan_shmem);
+
+		ret = acpm_achan_alloc_cmds(achan);
+		if (ret)
+			return ret;
+
+		spin_lock_init(&achan->rx_lock);
+		spin_lock_init(&achan->tx_lock);
+
+		cl->dev = dev;
+
+		achan->chan = mbox_request_channel(cl, achan->id);
+		if (IS_ERR(achan->chan)) {
+			acpm_free_mbox_chans(acpm);
+			return PTR_ERR(achan->chan);
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * acpm_setup_ops() - setup the operations structures.
+ * @acpm:	pointer to the driver data.
+ */
+static void acpm_setup_ops(struct acpm_info *acpm)
+{
+	struct acpm_pmic_ops *pmic_ops = &acpm->handle.ops.pmic_ops;
+
+	pmic_ops->read_reg = acpm_pmic_read_reg;
+	pmic_ops->bulk_read = acpm_pmic_bulk_read;
+	pmic_ops->write_reg = acpm_pmic_write_reg;
+	pmic_ops->bulk_write = acpm_pmic_bulk_write;
+	pmic_ops->update_reg = acpm_pmic_update_reg;
+}
+
+static int acpm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct device_node *shmem;
+	struct acpm_info *acpm;
+	resource_size_t size;
+	struct resource res;
+	const __be32 *prop;
+	int ret;
+
+	acpm = devm_kzalloc(dev, sizeof(*acpm), GFP_KERNEL);
+	if (!acpm)
+		return -ENOMEM;
+
+	shmem = of_parse_phandle(node, "shmem", 0);
+	ret = of_address_to_resource(shmem, 0, &res);
+	of_node_put(shmem);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to get shared memory.\n");
+
+	size = resource_size(&res);
+	acpm->sram_base = devm_ioremap(dev, res.start, size);
+	if (!acpm->sram_base)
+		return dev_err_probe(dev, -ENOMEM,
+				     "Failed to ioremap shared memory.\n");
+
+	prop = of_get_property(node, "initdata-base", NULL);
+	if (!prop)
+		return dev_err_probe(dev, -EINVAL,
+				     "Parsing initdata_base failed.\n");
+
+	acpm->dev = dev;
+	acpm->shmem = acpm->sram_base + be32_to_cpup(prop);
+
+	ret = acpm_channels_init(acpm);
+	if (ret)
+		return ret;
+
+	acpm_setup_ops(acpm);
+
+	INIT_LIST_HEAD(&acpm->node);
+	mutex_lock(&acpm_list_mutex);
+	list_add_tail(&acpm->node, &acpm_list);
+	mutex_unlock(&acpm_list_mutex);
+
+	platform_set_drvdata(pdev, acpm);
+
+	return 0;
+}
+
+static const struct of_device_id acpm_match[] = {
+	{ .compatible = "google,gs101-acpm-ipc" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, acpm_match);
+
+static struct platform_driver acpm_driver = {
+	.probe	= acpm_probe,
+	.driver	= {
+		.name = "exynos-acpm-protocol",
+		.of_match_table	= of_match_ptr(acpm_match),
+	},
+};
+module_platform_driver(acpm_driver);
+
+MODULE_AUTHOR("Tudor Ambarus <tudor.ambarus@linaro.org>");
+MODULE_DESCRIPTION("Samsung Exynos ACPM mailbox protocol driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/samsung/exynos-acpm.h b/drivers/firmware/samsung/exynos-acpm.h
new file mode 100644
index 000000000000..a03adcd260f5
--- /dev/null
+++ b/drivers/firmware/samsung/exynos-acpm.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright 2020 Samsung Electronics Co., Ltd.
+ * Copyright 2020 Google LLC.
+ * Copyright 2024 Linaro Ltd.
+ */
+#ifndef __EXYNOS_ACPM_H__
+#define __EXYNOS_ACPM_H__
+
+struct acpm_handle;
+struct acpm_xfer;
+
+int acpm_do_xfer(const struct acpm_handle *handle, struct acpm_xfer *xfer);
+
+#endif /* __EXYNOS_ACPM_H__ */
diff --git a/include/linux/soc/samsung/exynos-acpm-protocol.h b/include/linux/soc/samsung/exynos-acpm-protocol.h
new file mode 100644
index 000000000000..762783af7617
--- /dev/null
+++ b/include/linux/soc/samsung/exynos-acpm-protocol.h
@@ -0,0 +1,57 @@
+/* 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;
+	size_t len;
+};
+
+struct acpm_xfer {
+	struct acpm_msg tx;
+	struct acpm_msg rx;
+	int acpm_chan_id;
+#define ACPM_XFER_MAY_SLEEP     BIT(0)
+	u32 flags;
+};
+
+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;
+};
+
+struct device_node;
+
+const struct acpm_handle *devm_acpm_get_by_phandle(struct device *dev,
+						   const char *property);
+#endif /* __EXYNOS_ACPM_PROTOCOL_H */
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 2/3] firmware: add exynos ACPM protocol driver

[Daniel Lezcano]

On 12/5/24 18:53, Tudor Ambarus wrote:
> 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.
> 
> ACPM and masters regard each other as independent hardware component and
> communicate with each other using mailbox messages and shared memory.
> 
> This protocol library provides the interface for all the client drivers
> making use of the features offered by the APM. Add ACPM protocol support.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>   drivers/firmware/Kconfig                      |   1 +
>   drivers/firmware/Makefile                     |   1 +
>   drivers/firmware/samsung/Kconfig              |  14 +
>   drivers/firmware/samsung/Makefile             |   4 +
>   drivers/firmware/samsung/exynos-acpm-pmic.c   | 226 +++++
>   drivers/firmware/samsung/exynos-acpm-pmic.h   |  24 +
>   drivers/firmware/samsung/exynos-acpm.c        | 772 ++++++++++++++++++
>   drivers/firmware/samsung/exynos-acpm.h        |  15 +
>   .../linux/soc/samsung/exynos-acpm-protocol.h  |  57 ++
>   9 files changed, 1114 insertions(+)
>   create mode 100644 drivers/firmware/samsung/Kconfig
>   create mode 100644 drivers/firmware/samsung/Makefile
>   create mode 100644 drivers/firmware/samsung/exynos-acpm-pmic.c
>   create mode 100644 drivers/firmware/samsung/exynos-acpm-pmic.h
>   create mode 100644 drivers/firmware/samsung/exynos-acpm.c
>   create mode 100644 drivers/firmware/samsung/exynos-acpm.h
>   create mode 100644 include/linux/soc/samsung/exynos-acpm-protocol.h
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 71d8b26c4103..24edb956831b 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -267,6 +267,7 @@ source "drivers/firmware/meson/Kconfig"
>   source "drivers/firmware/microchip/Kconfig"
>   source "drivers/firmware/psci/Kconfig"
>   source "drivers/firmware/qcom/Kconfig"
> +source "drivers/firmware/samsung/Kconfig"
>   source "drivers/firmware/smccc/Kconfig"
>   source "drivers/firmware/tegra/Kconfig"
>   source "drivers/firmware/xilinx/Kconfig"
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 7a8d486e718f..91efcc868a05 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -33,6 +33,7 @@ obj-y				+= efi/
>   obj-y				+= imx/
>   obj-y				+= psci/
>   obj-y				+= qcom/
> +obj-y				+= samsung/
>   obj-y				+= smccc/
>   obj-y				+= tegra/
>   obj-y				+= xilinx/
> diff --git a/drivers/firmware/samsung/Kconfig b/drivers/firmware/samsung/Kconfig
> new file mode 100644
> index 000000000000..eed8cd8a677b
> --- /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"

Given the importance of this driver where a lot of PM services rely on, 
does it really make sense to allow it as a module ?

Some PM services may be needed very early in the boot process

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v3 2/3] firmware: add exynos ACPM protocol driver

[Krzysztof Kozlowski]

On Fri, Dec 06, 2024 at 12:39:56AM +0100, Daniel Lezcano wrote:
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +config EXYNOS_ACPM_PROTOCOL
> > +	tristate "Exynos Alive Clock and Power Manager (ACPM) Message Protocol"
> 
> Given the importance of this driver where a lot of PM services rely on, does
> it really make sense to allow it as a module ?
> 
> Some PM services may be needed very early in the boot process
> 

If it works as module e.g. on Android, it is beneficial. I think the
platform was booting fine without it, at least to some shell, so I can
imagine this can be loaded a bit later.

Best regards,
Krzysztof

Re: [PATCH v3 2/3] firmware: add exynos ACPM protocol driver

[Daniel Lezcano]

On 12/6/24 14:28, Krzysztof Kozlowski wrote:
> On Fri, Dec 06, 2024 at 12:39:56AM +0100, Daniel Lezcano wrote:
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +config EXYNOS_ACPM_PROTOCOL
>>> +	tristate "Exynos Alive Clock and Power Manager (ACPM) Message Protocol"
>>
>> Given the importance of this driver where a lot of PM services rely on, does
>> it really make sense to allow it as a module ?
>>
>> Some PM services may be needed very early in the boot process
>>
> 
> If it works as module e.g. on Android, it is beneficial. I think the
> platform was booting fine without it, at least to some shell, so I can
> imagine this can be loaded a bit later.

Usually the firmware sets the frequency to the maximum in order to boot 
the kernel as fast as possible. That may lead to thermal issues at boot 
time where the thermal framework won't be able to kick in as some 
components will depends on ACPM while the system stays at its highest 
performance state.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v3 2/3] firmware: add exynos ACPM protocol driver

[Markuss Broks]

On 12/6/24 9:50 PM, Daniel Lezcano wrote:
> On 12/6/24 14:28, Krzysztof Kozlowski wrote:
>> On Fri, Dec 06, 2024 at 12:39:56AM +0100, Daniel Lezcano wrote:
>>>> +# SPDX-License-Identifier: GPL-2.0-only
>>>> +
>>>> +config EXYNOS_ACPM_PROTOCOL
>>>> +    tristate "Exynos Alive Clock and Power Manager (ACPM) Message 
>>>> Protocol"
>>>
>>> Given the importance of this driver where a lot of PM services rely 
>>> on, does
>>> it really make sense to allow it as a module ?
>>>
>>> Some PM services may be needed very early in the boot process
>>>
>>
>> If it works as module e.g. on Android, it is beneficial. I think the
>> platform was booting fine without it, at least to some shell, so I can
>> imagine this can be loaded a bit later.
>
> Usually the firmware sets the frequency to the maximum in order to 
> boot the kernel as fast as possible. That may lead to thermal issues 
> at boot time where the thermal framework won't be able to kick in as 
> some components will depends on ACPM while the system stays at its 
> highest performance state.
Also, as far as I understand, ACPM is used here as an interface to the 
PMIC, so every driver which would need power management from the main 
SoC PMIC would get deferred until the ACPM module has been loaded. This 
would make it impossible to e.g. initialize the UFS or the MMC card 
before initramfs.
>
>
- Markuss

Re: [PATCH v3 2/3] firmware: add exynos ACPM protocol driver

[Krzysztof Kozlowski]

On Sun, Dec 08, 2024 at 06:38:50PM +0200, Markuss Broks wrote:
> 
> On 12/6/24 9:50 PM, Daniel Lezcano wrote:
> > On 12/6/24 14:28, Krzysztof Kozlowski wrote:
> > > On Fri, Dec 06, 2024 at 12:39:56AM +0100, Daniel Lezcano wrote:
> > > > > +# SPDX-License-Identifier: GPL-2.0-only
> > > > > +
> > > > > +config EXYNOS_ACPM_PROTOCOL
> > > > > +    tristate "Exynos Alive Clock and Power Manager (ACPM)
> > > > > Message Protocol"
> > > > 
> > > > Given the importance of this driver where a lot of PM services
> > > > rely on, does
> > > > it really make sense to allow it as a module ?
> > > > 
> > > > Some PM services may be needed very early in the boot process
> > > > 
> > > 
> > > If it works as module e.g. on Android, it is beneficial. I think the
> > > platform was booting fine without it, at least to some shell, so I can
> > > imagine this can be loaded a bit later.
> > 
> > Usually the firmware sets the frequency to the maximum in order to boot
> > the kernel as fast as possible. That may lead to thermal issues at boot
> > time where the thermal framework won't be able to kick in as some
> > components will depends on ACPM while the system stays at its highest
> > performance state.

I disagree with the first assumption: usually firmware selects high, but
safe frequency. Otherwise you would not be able to wait in bootloader
prompt.

> Also, as far as I understand, ACPM is used here as an interface to the PMIC,
> so every driver which would need power management from the main SoC PMIC
> would get deferred until the ACPM module has been loaded. This would make it

It was an issue 10 years ago, not anymore. Drivers handle deferred
probe.

> impossible to e.g. initialize the UFS or the MMC card before initramfs.

Which is not a problem, because you are supposed to have initramfs. This
is preferred way. Being this a module does not force you to use it as a
module, e.g. if in your setup you do not have initramfs (although it is
unlikely for arm64 platforms...).

Best regards,
Krzysztof

Re: [PATCH v3 2/3] firmware: add exynos ACPM protocol driver

[Peter Griffin]

Hi Daniel,

Thanks for your review feedback.

On Fri, 6 Dec 2024 at 19:50, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 12/6/24 14:28, Krzysztof Kozlowski wrote:
> > On Fri, Dec 06, 2024 at 12:39:56AM +0100, Daniel Lezcano wrote:
> >>> +# SPDX-License-Identifier: GPL-2.0-only
> >>> +
> >>> +config EXYNOS_ACPM_PROTOCOL
> >>> +   tristate "Exynos Alive Clock and Power Manager (ACPM) Message Protocol"
> >>
> >> Given the importance of this driver where a lot of PM services rely on, does
> >> it really make sense to allow it as a module ?

Yes, we want the option to build it as a module so we can use the
upstreamed driver with Generic Kernel Image (GKI) [1].

> >>
> >> Some PM services may be needed very early in the boot process
> >>
> >
> > If it works as module e.g. on Android, it is beneficial. I think the
> > platform was booting fine without it, at least to some shell, so I can
> > imagine this can be loaded a bit later.
>
> Usually the firmware sets the frequency to the maximum in order to boot
> the kernel as fast as possible. That may lead to thermal issues at boot
> time where the thermal framework won't be able to kick in as some
> components will depends on ACPM while the system stays at its highest
> performance state.

That isn't an issue here as the Pixel 6 bootloader leaves CPUs at mid
point frequencies during boot. I would actually expect most modern
phone bootloaders (since the launch of GKI at least) to do something
similar as it is a requirement for Generic Kernel Image (GKI) [1] that
all the SoC drivers are built as modules.

[1] https://source.android.com/docs/core/architecture/kernel/generic-kernel-image

Thanks,

Peter

Re: [PATCH v3 2/3] firmware: add exynos ACPM protocol driver

[Arnd Bergmann]

On Thu, Dec 5, 2024, at 18:53, Tudor Ambarus wrote:

> +#define exynos_acpm_set_bulk(data, i)					\
> +	(((data) & ACPM_BULK_MASK) << (ACPM_BULK_SHIFT * (i)))
> +#define exynos_acpm_read_bulk(data, i)					\
> +	(((data) >> (ACPM_BULK_SHIFT * (i))) & ACPM_BULK_MASK)

Could these be inline functions for readability?

> +	cmd[3] = (u32)(sched_clock() / 1000000); /*record ktime ms*/

The comment does not match the implementation, sched_clock()
is probably not what you want here because of its limitiations.

Maybe ktime_to_ms(ktime_get())?

> +/**
> + * acpm_get_rx() - get response from RX queue.
> + * @achan:	ACPM channel info.
> + * @xfer:	reference to the transfer to get response for.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int acpm_get_rx(struct acpm_chan *achan, struct acpm_xfer *xfer)
> +{
> +	struct acpm_msg *tx = &xfer->tx;
> +	struct acpm_msg *rx = &xfer->rx;
> +	struct acpm_rx_data *rx_data;
> +	const void __iomem *base, *addr;
> +	u32 rx_front, rx_seqnum, tx_seqnum, seqnum;
> +	u32 i, val, mlen;
> +	bool rx_set = false;
> +
> +	rx_front = readl_relaxed(achan->rx.front);
> +	i = readl_relaxed(achan->rx.rear);

If you have to use readl_relaxed(), please annotate why,
otherwise just use the normal readl().  Is this access to
the SRAM?

> +	spin_lock_irqsave(&achan->tx_lock, flags);
> +
> +	tx_front = readl_relaxed(achan->tx.front);
> +	idx = (tx_front + 1) % achan->qlen;
> +
> +	ret = acpm_wait_for_queue_slots(achan, idx);
> +	if (ret) {
> +		spin_unlock_irqrestore(&achan->tx_lock, flags);
> +		return ret;
> +	}

It looks like you are calling a busy loop function inside
of a hardirq handler here, with a 500ms timeout. This is
not ok.

If you may need to wait for a long timeout, I would suggest
changing the interface so that this function is not allowed
to be called from irq-disabled context, change the spinlock
to a mutex and polling read to a sleeping version.

> +	/* Advance TX front. */
> +	writel_relaxed(idx, achan->tx.front);
> +
> +	/* Flush SRAM posted writes. */
> +	readl_relaxed(achan->tx.front);
> +
> +	spin_unlock_irqrestore(&achan->tx_lock, flags);

I don't think this sequence guarantees the serialization
you want. By making the access _relaxed() you explicitly
say you don't want serialization, so the store does
not have to complete before the unlock.

> +static const struct of_device_id acpm_match[] = {
> +	{ .compatible = "google,gs101-acpm-ipc" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, acpm_match);
> +
> +static struct platform_driver acpm_driver = {
> +	.probe	= acpm_probe,
> +	.driver	= {
> +		.name = "exynos-acpm-protocol",
> +		.of_match_table	= of_match_ptr(acpm_match),

Remove the stray of_match_ptr() here.

> diff --git a/drivers/firmware/samsung/exynos-acpm.h 
> b/drivers/firmware/samsung/exynos-acpm.h
> new file mode 100644
> index 000000000000..a03adcd260f5
> --- /dev/null
> +++ b/drivers/firmware/samsung/exynos-acpm.h
> @@ -0,0 +1,15 @@
> +#ifndef __EXYNOS_ACPM_H__
> +#define __EXYNOS_ACPM_H__
> +
> +struct acpm_handle;
> +struct acpm_xfer;
> +
> +int acpm_do_xfer(const struct acpm_handle *handle, struct acpm_xfer 
> *xfer);
> +
> +#endif /* __EXYNOS_ACPM_H__ */
> diff --git a/include/linux/soc/samsung/exynos-acpm-protocol.h 
> b/include/linux/soc/samsung/exynos-acpm-protocol.h
> new file mode 100644
> index 000000000000..762783af7617
> --- /dev/null
> +++ b/include/linux/soc/samsung/exynos-acpm-protocol.h

Why is this in include/linux/soc, and not in the firmware
header?

      Arnd

Re: [PATCH v3 2/3] firmware: add exynos ACPM protocol driver

[Tudor Ambarus]

Thanks for the review, Arnd!

On 12/6/24 6:47 AM, Arnd Bergmann wrote:
> On Thu, Dec 5, 2024, at 18:53, Tudor Ambarus wrote:
> 
>> +#define exynos_acpm_set_bulk(data, i)					\
>> +	(((data) & ACPM_BULK_MASK) << (ACPM_BULK_SHIFT * (i)))
>> +#define exynos_acpm_read_bulk(data, i)					\
>> +	(((data) >> (ACPM_BULK_SHIFT * (i))) & ACPM_BULK_MASK)
> 
> Could these be inline functions for readability?

sure, will do.

> 
>> +	cmd[3] = (u32)(sched_clock() / 1000000); /*record ktime ms*/
> 
> The comment does not match the implementation, sched_clock()
> is probably not what you want here because of its limitiations.
> 
> Maybe ktime_to_ms(ktime_get())?
> 

Indeed, will use, thanks.

>> +/**
>> + * acpm_get_rx() - get response from RX queue.
>> + * @achan:	ACPM channel info.
>> + * @xfer:	reference to the transfer to get response for.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int acpm_get_rx(struct acpm_chan *achan, struct acpm_xfer *xfer)
>> +{
>> +	struct acpm_msg *tx = &xfer->tx;
>> +	struct acpm_msg *rx = &xfer->rx;
>> +	struct acpm_rx_data *rx_data;
>> +	const void __iomem *base, *addr;
>> +	u32 rx_front, rx_seqnum, tx_seqnum, seqnum;
>> +	u32 i, val, mlen;
>> +	bool rx_set = false;
>> +
>> +	rx_front = readl_relaxed(achan->rx.front);
>> +	i = readl_relaxed(achan->rx.rear);
> 
> If you have to use readl_relaxed(), please annotate why,
> otherwise just use the normal readl().  Is this access to
> the SRAM?

all IO accesses in this driver are to SRAM, yes.

There are no DMA accesses involved in the driver and the _relaxed()
accessors are fully ordered for accesses to the same endpoint, so I
thought _relaxed are fine.

> 
>> +	spin_lock_irqsave(&achan->tx_lock, flags);
>> +
>> +	tx_front = readl_relaxed(achan->tx.front);
>> +	idx = (tx_front + 1) % achan->qlen;
>> +
>> +	ret = acpm_wait_for_queue_slots(achan, idx);
>> +	if (ret) {
>> +		spin_unlock_irqrestore(&achan->tx_lock, flags);
>> +		return ret;
>> +	}
> 
> It looks like you are calling a busy loop function inside
> of a hardirq handler here, with a 500ms timeout. This is
> not ok.

That's true, the code assumes that the method can be called from hard
irq context.

Can't tell whether that timeout is accurate, it comes from downstream
and the resources that I have do not specify anything about what would
be an acceptable timeout.

I see arm_scmi typically uses 30 ms for transport layers.

> 
> If you may need to wait for a long timeout, I would suggest
> changing the interface so that this function is not allowed
> to be called from irq-disabled context, change the spinlock
> to a mutex and polling read to a sleeping version.

I think the question I need to answer here is whether the acpm interface
can be called from atomic context or not. On a first look, all
downstream drivers call it from process context. Curios there's no clock
enable though in downstream, which would require atomic context. I'll
get back to you on this.

If there's at least a client that calls the interface in hard/soft irq
context (clocks?) then I don't think I'll be able to implement your
suggestion. Each channel has its own TX/RX rings in SRAM. If there are
requests from both hard irq and process context for the same channel,
then I need to protect the accesses to the rings via spin_lock_irqsave.
This is what the code assumes, because downstream allows calls from
atomic context even if I can't pinpoint one right now.

I guess I can switch everything to sleeping version, and worry about
atomic context when such a client gets proposed?

> 
>> +	/* Advance TX front. */
>> +	writel_relaxed(idx, achan->tx.front);
>> +
>> +	/* Flush SRAM posted writes. */
>> +	readl_relaxed(achan->tx.front);
>> +
>> +	spin_unlock_irqrestore(&achan->tx_lock, flags);
> 
> I don't think this sequence guarantees the serialization
> you want. By making the access _relaxed() you explicitly
> say you don't want serialization, so the store does
> not have to complete before the unlock.
> 

I could benefit of the non relaxed versions indeed.


>> +		.of_match_table	= of_match_ptr(acpm_match),
> 
> Remove the stray of_match_ptr() here.

okay

>> --- /dev/null
>> +++ b/include/linux/soc/samsung/exynos-acpm-protocol.h
> 
> Why is this in include/linux/soc, and not in the firmware
> header?

right, will move.

Thanks!
ta

Re: [PATCH v3 2/3] firmware: add exynos ACPM protocol driver

[Arnd Bergmann]

On Fri, Dec 6, 2024, at 12:59, Tudor Ambarus wrote:
>>> +/**
>>> + * acpm_get_rx() - get response from RX queue.
>>> + * @achan:	ACPM channel info.
>>> + * @xfer:	reference to the transfer to get response for.
>>> + *
>>> + * Return: 0 on success, -errno otherwise.
>>> + */
>>> +static int acpm_get_rx(struct acpm_chan *achan, struct acpm_xfer *xfer)
>>> +{
>>> +	struct acpm_msg *tx = &xfer->tx;
>>> +	struct acpm_msg *rx = &xfer->rx;
>>> +	struct acpm_rx_data *rx_data;
>>> +	const void __iomem *base, *addr;
>>> +	u32 rx_front, rx_seqnum, tx_seqnum, seqnum;
>>> +	u32 i, val, mlen;
>>> +	bool rx_set = false;
>>> +
>>> +	rx_front = readl_relaxed(achan->rx.front);
>>> +	i = readl_relaxed(achan->rx.rear);
>> 
>> If you have to use readl_relaxed(), please annotate why,
>> otherwise just use the normal readl().  Is this access to
>> the SRAM?
>
> all IO accesses in this driver are to SRAM, yes.
>
> There are no DMA accesses involved in the driver and the _relaxed()
> accessors are fully ordered for accesses to the same endpoint, so I
> thought _relaxed are fine.

My normal rule is to only use _relaxed() if it makes any
intended difference in performance or behavior, otherwise
every reader of the code has to understand why exactly this
was done. More generally speaking, use the function with the
shortest name to do what you want, since the longer functions
are usually special cases that are not the default ;-)

SRAM is a bit special, as you don't have to deal with
side-effects the same way that an MMIO register would,
though there is still a question of serializing against
concurrent accesses from another processor.

It's possible that readl_relaxed()/writel_relaxed() end
up being exactly what you wand for this driver, but I would
probably encapsulate them in a local sram_readl()/sram_writel()
wrapper along with some documentation.

You should also have a look at which ioremap() version
you actually want. The default ioremap() uses
PROT_DEVICE_nGnRE and has strong ordering but not the
"non-posted" guarantee like ioremap_np() that would
let you skip the read-after-write for serialization.

If you care a lot about performance on the SRAM access,
you can go the other way with a more relaxed ioremap_wc()
and explicit barriers where needed, or even better
see what the remote side uses and use the same mapping
flags and serialization here.

>>> +	spin_lock_irqsave(&achan->tx_lock, flags);
>>> +
>>> +	tx_front = readl_relaxed(achan->tx.front);
>>> +	idx = (tx_front + 1) % achan->qlen;
>>> +
>>> +	ret = acpm_wait_for_queue_slots(achan, idx);
>>> +	if (ret) {
>>> +		spin_unlock_irqrestore(&achan->tx_lock, flags);
>>> +		return ret;
>>> +	}
>> 
>> It looks like you are calling a busy loop function inside
>> of a hardirq handler here, with a 500ms timeout. This is
>> not ok.
>
> That's true, the code assumes that the method can be called from hard
> irq context.
>
> Can't tell whether that timeout is accurate, it comes from downstream
> and the resources that I have do not specify anything about what would
> be an acceptable timeout.
>
> I see arm_scmi typically uses 30 ms for transport layers.

I think 30ms is still too much for a busy loop. I see that in
the SCMI driver it implements both a sleeping and a spinning
loop, but I have not found exactly which callers use the
spinning version, and what timeouts those have.

>> If you may need to wait for a long timeout, I would suggest
>> changing the interface so that this function is not allowed
>> to be called from irq-disabled context, change the spinlock
>> to a mutex and polling read to a sleeping version.
>
> I think the question I need to answer here is whether the acpm interface
> can be called from atomic context or not. On a first look, all
> downstream drivers call it from process context. Curios there's no clock
> enable though in downstream, which would require atomic context. I'll
> get back to you on this.
>
> If there's at least a client that calls the interface in hard/soft irq
> context (clocks?) then I don't think I'll be able to implement your
> suggestion. Each channel has its own TX/RX rings in SRAM. If there are
> requests from both hard irq and process context for the same channel,
> then I need to protect the accesses to the rings via spin_lock_irqsave.
> This is what the code assumes, because downstream allows calls from
> atomic context even if I can't pinpoint one right now.
>
> I guess I can switch everything to sleeping version, and worry about
> atomic context when such a client gets proposed?

Yes, usually at that point the caller can be changed to use
a threaded IRQ or similar.

      Arnd

[PATCH v3 3/3] MAINTAINERS: add entry for the samsung exynos ACPM mailbox protocol

[Tudor Ambarus]

Add entry for the samsung exynos ACPM mailbox protocol.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1e930c7a58b1..5cff01641f23 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3023,6 +3023,7 @@ F:	drivers/*/*s3c24*
 F:	drivers/*/*s3c64xx*
 F:	drivers/*/*s5pv210*
 F:	drivers/clocksource/samsung_pwm_timer.c
+F:	drivers/firmware/samsung/
 F:	drivers/memory/samsung/
 F:	drivers/pwm/pwm-samsung.c
 F:	drivers/soc/samsung/
@@ -20712,6 +20713,15 @@ F:	arch/arm64/boot/dts/exynos/exynos850*
 F:	drivers/clk/samsung/clk-exynos850.c
 F:	include/dt-bindings/clock/exynos850.h
 
+SAMSUNG EXYNOS ACPM MAILBOX PROTOCOL
+M:	Tudor Ambarus <tudor.ambarus@linaro.org>
+L:	linux-kernel@vger.kernel.org
+L:	linux-samsung-soc@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/firmware/samsung,exynos-acpm-ipc.yaml
+F:	drivers/firmware/samsung/exynos-acpm*
+F:	include/linux/soc/samsung/exynos-acpm-protocol.h
+
 SAMSUNG EXYNOS PSEUDO RANDOM NUMBER GENERATOR (RNG) DRIVER
 M:	Krzysztof Kozlowski <krzk@kernel.org>
 L:	linux-crypto@vger.kernel.org
-- 
2.47.0.338.g60cca15819-goog