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.
The mailbox channels are initialized based on the configuration data
found at a specific offset into the shared memory (mmio-sram). The
configuration data consists of channel id, message and queue lengths,
pointers to the RX and TX queues which are also part of the SRAM, and
whether RX works by polling or interrupts. All known clients of this
driver are using polling channels, thus the driver implements for now
just polling mode.
Add support for the exynos acpm core driver. Helper drivers will follow.
These will construct the mailbox messages in the format expected by the
firmware.
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
drivers/firmware/Kconfig | 1 +
drivers/firmware/Makefile | 1 +
drivers/firmware/samsung/Kconfig | 11 +
drivers/firmware/samsung/Makefile | 3 +
drivers/firmware/samsung/exynos-acpm.c | 703 ++++++++++++++++++++
include/linux/mailbox/exynos-acpm-message.h | 21 +
6 files changed, 740 insertions(+)
create mode 100644 drivers/firmware/samsung/Kconfig
create mode 100644 drivers/firmware/samsung/Makefile
create mode 100644 drivers/firmware/samsung/exynos-acpm.c
create mode 100644 include/linux/mailbox/exynos-acpm-message.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..f908773c1441
--- /dev/null
+++ b/drivers/firmware/samsung/Kconfig
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+config EXYNOS_ACPM
+ tristate "Exynos ACPM (Alive Clock and Power Manager) driver support"
+ select MAILBOX
+ help
+ ACPM 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 module provides the means to communicate with the ACPM firmware.
diff --git a/drivers/firmware/samsung/Makefile b/drivers/firmware/samsung/Makefile
new file mode 100644
index 000000000000..35ff3076bbea
--- /dev/null
+++ b/drivers/firmware/samsung/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+obj-$(CONFIG_EXYNOS_ACPM) += exynos-acpm.o
diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
new file mode 100644
index 000000000000..c3ad4dc7a9e0
--- /dev/null
+++ b/drivers/firmware/samsung/exynos-acpm.c
@@ -0,0 +1,703 @@
+// 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/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox/exynos-acpm-message.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define EXYNOS_ACPM_MCUCTRL 0x0 /* Mailbox Control Register */
+#define EXYNOS_ACPM_INTCR0 0x24 /* Interrupt Clear Register 0 */
+#define EXYNOS_ACPM_INTMR0 0x28 /* Interrupt Mask Register 0 */
+#define EXYNOS_ACPM_INTSR0 0x2c /* Interrupt Status Register 0 */
+#define EXYNOS_ACPM_INTMSR0 0x30 /* Interrupt Mask Status Register 0 */
+#define EXYNOS_ACPM_INTGR1 0x40 /* Interrupt Generation Register 1 */
+#define EXYNOS_ACPM_INTMR1 0x48 /* Interrupt Mask Register 1 */
+#define EXYNOS_ACPM_INTSR1 0x4c /* Interrupt Status Register 1 */
+#define EXYNOS_ACPM_INTMSR1 0x50 /* Interrupt Mask Status Register 1 */
+
+#define EXYNOS_ACPM_INTMR0_MASK GENMASK(15, 0)
+#define EXYNOS_ACPM_PROTOCOL_SEQNUM GENMASK(21, 16)
+
+/* The unit of counter is 20 us. 5000 * 20 = 100 ms */
+#define EXYNOS_ACPM_POLL_TIMEOUT 5000
+#define EXYNOS_ACPM_TX_TIMEOUT_US 500000
+
+/**
+ * struct exynos_acpm_shmem - mailbox shared memory configuration information.
+ * @reserved: reserved for future use.
+ * @chans: offset to array of struct exynos_acpm_shmem_chan.
+ * @reserved1: reserved for future use.
+ * @num_chans: number of channels.
+ */
+struct exynos_acpm_shmem {
+ u32 reserved[2];
+ u32 chans;
+ u32 reserved1[3];
+ u32 num_chans;
+};
+
+/**
+ * struct exynos_acpm_shmem_chan - descriptor of a shared memory channel.
+ *
+ * @id: channel ID.
+ * @reserved: reserved for future use.
+ * @rx_rear: rear pointer of RX queue.
+ * @rx_front: front pointer of RX queue.
+ * @rx_base: base address of RX queue.
+ * @reserved1: reserved for future use.
+ * @tx_rear: rear pointer of TX queue.
+ * @tx_front: front pointer of TX queue.
+ * @tx_base: base address of TX queue.
+ * @qlen: queue length. Applies to both TX/RX queues.
+ * @mlen: message length. Applies to both TX/RX queues.
+ * @reserved2: reserved for future use.
+ * @polling: true when the channel works on polling.
+ */
+struct exynos_acpm_shmem_chan {
+ 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 polling;
+};
+
+/**
+ * struct exynos_acpm_queue - exynos acpm queue.
+ *
+ * @rear: rear address of the queue.
+ * @front: front address of the queue.
+ * @base: base address of the queue.
+ */
+struct exynos_acpm_queue {
+ void __iomem *rear;
+ void __iomem *front;
+ void __iomem *base;
+};
+
+/**
+ * struct exynos_acpm_rx_data - RX queue data.
+ *
+ * @cmd: pointer to where the data shall be saved.
+ * @response: true if the client expects the RX data.
+ */
+struct exynos_acpm_rx_data {
+ u32 *cmd;
+ bool response;
+};
+
+#define EXYNOS_ACPM_SEQNUM_MAX 64
+
+/**
+ * struct exynos_acpm_chan - driver internal representation of a channel.
+ * @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.
+ * @rx_lock: protects RX queue. The RX queue is accessed just in
+ * process context.
+ * @tx_lock: protects TX 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.
+ * @polling: true when the channel works on polling.
+ * @bitmap_seqnum: bitmap that tracks the messages on the TX/RX queues.
+ * @rx_data: internal buffer used to drain the RX queue.
+ */
+struct exynos_acpm_chan {
+ struct exynos_acpm_queue tx;
+ struct exynos_acpm_queue rx;
+ struct mutex rx_lock;
+ spinlock_t tx_lock;
+
+ unsigned int qlen;
+ unsigned int mlen;
+ u8 seqnum;
+ u8 id;
+ bool polling;
+
+ DECLARE_BITMAP(bitmap_seqnum, EXYNOS_ACPM_SEQNUM_MAX - 1);
+ struct exynos_acpm_rx_data rx_data[EXYNOS_ACPM_SEQNUM_MAX];
+};
+
+/**
+ * struct exynos_acpm - driver's private data.
+ * @shmem: pointer to the SRAM configuration data.
+ * @chans: pointer to the ACPM channel parameters retrieved from SRAM.
+ * @sram_base: base address of SRAM.
+ * @regs: mailbox registers base address.
+ * @mbox: pointer to the mailbox controller.
+ * @wq: pointer to workqueue.
+ * @dev: pointer to the exynos-acpm device.
+ * @pclk: pointer to the mailbox peripheral clock.
+ * @num_chans: number of channels available for this controller.
+ */
+struct exynos_acpm {
+ struct exynos_acpm_shmem *shmem;
+ struct exynos_acpm_chan *chans;
+ void __iomem *sram_base;
+ void __iomem *regs;
+ struct mbox_controller *mbox;
+ struct workqueue_struct *wq;
+ struct device *dev;
+ struct clk *pclk;
+ u32 num_chans;
+};
+
+/**
+ * struct exynos_acpm_work_data - data structure representing the work.
+ * @mbox_chan: pointer to the mailbox channel.
+ * @req: pointer to the mailbox request.
+ * @callback: pointer to a callback function to be invoked upon
+ * completion of this request.
+ * @work: describes the task to be executed.
+ */
+struct exynos_acpm_work_data {
+ struct mbox_chan *mbox_chan;
+ struct mbox_request *req;
+ void (*callback)(struct exynos_acpm_work_data *work_data, int status);
+ struct work_struct work;
+};
+
+static int exynos_acpm_get_rx(struct mbox_chan *mbox_chan,
+ struct mbox_request *req)
+{
+ struct exynos_acpm_chan *chan = mbox_chan->con_priv;
+ struct exynos_acpm_message *tx = req->tx;
+ struct exynos_acpm_message *rx = req->rx;
+ struct exynos_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(chan->rx.front);
+ i = readl_relaxed(chan->rx.rear);
+
+ /* Bail out if RX is empty. */
+ if (i == rx_front)
+ return 0;
+
+ base = chan->rx.base;
+ mlen = chan->mlen;
+
+ tx_seqnum = FIELD_GET(EXYNOS_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(EXYNOS_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 = &chan->rx_data[seqnum];
+
+ if (rx_data->response) {
+ if (rx_seqnum == tx_seqnum) {
+ __ioread32_copy(rx->cmd, addr, req->rxlen / 4);
+ rx_set = true;
+ clear_bit(seqnum, chan->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,
+ req->rxlen / 4);
+ }
+ } else {
+ clear_bit(seqnum, chan->bitmap_seqnum);
+ }
+
+ i = (i + 1) % chan->qlen;
+ } while (i != rx_front);
+
+ /* We saved all responses, mark RX empty. */
+ writel_relaxed(rx_front, chan->rx.rear);
+
+ /* Flush SRAM posted writes. */
+ readl_relaxed(chan->rx.front);
+
+ /*
+ * If the response was not in this iteration of the queue, check if the
+ * RX data was previously saved.
+ */
+ rx_data = &chan->rx_data[tx_seqnum - 1];
+ if (!rx_set && rx_data->response) {
+ rx_seqnum = FIELD_GET(EXYNOS_ACPM_PROTOCOL_SEQNUM,
+ rx_data->cmd[0]);
+
+ if (rx_seqnum == tx_seqnum) {
+ memcpy(rx->cmd, rx_data->cmd, req->rxlen);
+ clear_bit(rx_seqnum - 1, chan->bitmap_seqnum);
+ }
+ }
+
+ return 0;
+}
+
+static int exynos_acpm_dequeue_by_polling(struct mbox_chan *mbox_chan,
+ struct mbox_request *req)
+{
+ struct exynos_acpm_chan *chan = mbox_chan->con_priv;
+ struct exynos_acpm_message *tx = req->tx;
+ struct device *dev = mbox_chan->mbox->dev;
+ unsigned int cnt_20us = 0;
+ u32 seqnum;
+ int ret;
+
+ seqnum = FIELD_GET(EXYNOS_ACPM_PROTOCOL_SEQNUM, tx->cmd[0]);
+
+ do {
+ ret = mutex_lock_interruptible(&chan->rx_lock);
+ if (ret)
+ return ret;
+ ret = exynos_acpm_get_rx(mbox_chan, req);
+ mutex_unlock(&chan->rx_lock);
+ if (ret)
+ return ret;
+
+ if (!test_bit(seqnum - 1, chan->bitmap_seqnum)) {
+ dev_vdbg(dev, "cnt_20us = %d.\n", cnt_20us);
+ return 0;
+ }
+
+ /* Determined experimentally. */
+ usleep_range(20, 30);
+ cnt_20us++;
+ } while (cnt_20us < EXYNOS_ACPM_POLL_TIMEOUT);
+
+ dev_err(dev, "Timeout! ch:%u s:%u bitmap:%lx, cnt_20us = %d.\n",
+ chan->id, seqnum, chan->bitmap_seqnum[0], cnt_20us);
+
+ return -ETIME;
+}
+
+static void exynos_acpm_done(struct exynos_acpm_work_data *work_data, int status)
+{
+ struct mbox_request *req = work_data->req;
+
+ kfree(work_data);
+ mbox_request_complete(req, status);
+}
+
+static void exynos_acpm_work_handler(struct work_struct *work)
+{
+ struct exynos_acpm_work_data *work_data =
+ container_of(work, struct exynos_acpm_work_data, work);
+ struct mbox_chan *mbox_chan = work_data->mbox_chan;
+ int ret;
+
+ ret = exynos_acpm_dequeue_by_polling(mbox_chan, work_data->req);
+ work_data->callback(work_data, ret);
+}
+
+static struct exynos_acpm_work_data *
+ exynos_acpm_init_work(struct mbox_chan *mbox_chan,
+ struct mbox_request *req)
+{
+ struct exynos_acpm_work_data *work_data;
+ gfp_t gfp = (req->flags & MBOX_REQ_MAY_SLEEP) ? GFP_KERNEL : GFP_ATOMIC;
+
+ work_data = kmalloc(sizeof(*work_data), gfp);
+ if (!work_data)
+ return ERR_PTR(-ENOMEM);
+
+ work_data->mbox_chan = mbox_chan;
+ work_data->req = req;
+ work_data->callback = exynos_acpm_done;
+ INIT_WORK(&work_data->work, exynos_acpm_work_handler);
+
+ return work_data;
+}
+
+static void exynos_acpm_prepare_request(struct mbox_chan *mbox_chan,
+ struct mbox_request *req)
+{
+ struct exynos_acpm_chan *chan = mbox_chan->con_priv;
+ struct exynos_acpm_message *tx = req->tx;
+ struct exynos_acpm_rx_data *rx_data;
+
+ /* Prevent chan->seqnum from being re-used */
+ do {
+ if (++chan->seqnum == EXYNOS_ACPM_SEQNUM_MAX)
+ chan->seqnum = 1;
+ } while (test_bit(chan->seqnum - 1, chan->bitmap_seqnum));
+
+ tx->cmd[0] |= FIELD_PREP(EXYNOS_ACPM_PROTOCOL_SEQNUM, chan->seqnum);
+
+ /* Clear data for upcoming responses */
+ rx_data = &chan->rx_data[chan->seqnum - 1];
+ memset(rx_data->cmd, 0, sizeof(*(rx_data->cmd)) * chan->mlen);
+ if (req->rx)
+ rx_data->response = true;
+
+ /* Flag the index based on seqnum. (seqnum: 1~63, bitmap: 0~62) */
+ set_bit(chan->seqnum - 1, chan->bitmap_seqnum);
+}
+
+static int exynos_acpm_wait_for_queue_slots(struct mbox_chan *mbox_chan,
+ u32 next_tx_front)
+{
+ struct exynos_acpm_chan *chan = mbox_chan->con_priv;
+ struct device *dev = mbox_chan->mbox->dev;
+ 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(chan->rx.front, val,
+ next_tx_front != val, 1,
+ EXYNOS_ACPM_TX_TIMEOUT_US);
+ if (ret) {
+ dev_err(dev, "RX front can not keep up with TX front.\n");
+ return ret;
+ }
+
+ ret = readl_relaxed_poll_timeout_atomic(chan->tx.rear, val,
+ next_tx_front != val, 1,
+ EXYNOS_ACPM_TX_TIMEOUT_US);
+ if (ret)
+ dev_err(dev, "TX queue is full.\n");
+
+ return ret;
+}
+
+static int exynos_acpm_send_request(struct mbox_chan *mbox_chan,
+ struct mbox_request *req)
+{
+ struct exynos_acpm *exynos_acpm = dev_get_drvdata(mbox_chan->mbox->dev);
+ struct exynos_acpm_chan *chan = mbox_chan->con_priv;
+ struct exynos_acpm_message *tx = req->tx;
+ struct exynos_acpm_work_data *work_data;
+ u32 idx, tx_front;
+ unsigned long flags;
+ int ret;
+
+ if (!tx || !tx->cmd || req->txlen > chan->mlen ||
+ req->rxlen > chan->mlen)
+ return -EINVAL;
+
+ work_data = exynos_acpm_init_work(mbox_chan, req);
+ if (IS_ERR(work_data))
+ return PTR_ERR(work_data);
+
+ spin_lock_irqsave(&chan->tx_lock, flags);
+
+ tx_front = readl_relaxed(chan->tx.front);
+ idx = (tx_front + 1) % chan->qlen;
+
+ ret = exynos_acpm_wait_for_queue_slots(mbox_chan, idx);
+ if (ret)
+ goto exit;
+
+ exynos_acpm_prepare_request(mbox_chan, req);
+
+ /* Write TX command. */
+ __iowrite32_copy(chan->tx.base + chan->mlen * tx_front, tx->cmd,
+ req->txlen / 4);
+
+ /* Advance TX front. */
+ writel_relaxed(idx, chan->tx.front);
+
+ /* Flush SRAM posted writes. */
+ readl_relaxed(chan->tx.front);
+
+ /* Generate ACPM interrupt. */
+ writel_relaxed(BIT(chan->id), exynos_acpm->regs + EXYNOS_ACPM_INTGR1);
+
+ /* Flush mailbox controller posted writes. */
+ readl_relaxed(exynos_acpm->regs + EXYNOS_ACPM_MCUCTRL);
+
+ spin_unlock_irqrestore(&chan->tx_lock, flags);
+
+ queue_work(exynos_acpm->wq, &work_data->work);
+
+ return -EINPROGRESS;
+exit:
+ spin_unlock_irqrestore(&chan->tx_lock, flags);
+ kfree(work_data);
+ return ret;
+}
+
+static int exynos_acpm_chan_startup(struct mbox_chan *mbox_chan)
+{
+ struct exynos_acpm_chan *chan = mbox_chan->con_priv;
+
+ if (!chan->polling) {
+ dev_err(mbox_chan->mbox->dev, "IRQs not supported.\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static const struct mbox_chan_ops exynos_acpm_chan_ops = {
+ .send_request = exynos_acpm_send_request,
+ .startup = exynos_acpm_chan_startup,
+};
+
+static void __iomem *exynos_acpm_get_iomem_addr(void __iomem *base,
+ void __iomem *addr)
+{
+ u32 offset;
+
+ offset = readl_relaxed(addr);
+ return base + offset;
+}
+
+static void exynos_acpm_rx_queue_init(struct exynos_acpm *exynos_acpm,
+ struct exynos_acpm_shmem_chan *shmem_chan,
+ struct exynos_acpm_queue *rx)
+{
+ void __iomem *base = exynos_acpm->sram_base;
+
+ rx->base = exynos_acpm_get_iomem_addr(base, &shmem_chan->tx_base);
+ rx->front = exynos_acpm_get_iomem_addr(base, &shmem_chan->tx_front);
+ rx->rear = exynos_acpm_get_iomem_addr(base, &shmem_chan->tx_rear);
+}
+
+static void exynos_acpm_tx_queue_init(struct exynos_acpm *exynos_acpm,
+ struct exynos_acpm_shmem_chan *shmem_chan,
+ struct exynos_acpm_queue *tx)
+{
+ void __iomem *base = exynos_acpm->sram_base;
+
+ tx->base = exynos_acpm_get_iomem_addr(base, &shmem_chan->rx_base);
+ tx->front = exynos_acpm_get_iomem_addr(base, &shmem_chan->rx_front);
+ tx->rear = exynos_acpm_get_iomem_addr(base, &shmem_chan->rx_rear);
+}
+
+static int exynos_acpm_alloc_cmds(struct exynos_acpm *exynos_acpm,
+ struct exynos_acpm_chan *chan)
+{
+ struct device *dev = exynos_acpm->dev;
+ struct exynos_acpm_rx_data *rx_data;
+ unsigned int mlen = chan->mlen;
+ int i;
+
+ for (i = 0; i < EXYNOS_ACPM_SEQNUM_MAX; i++) {
+ rx_data = &chan->rx_data[i];
+ rx_data->cmd = devm_kcalloc(dev, mlen, sizeof(*(rx_data->cmd)),
+ GFP_KERNEL);
+ if (!rx_data->cmd)
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static int exynos_acpm_chans_init(struct exynos_acpm *exynos_acpm)
+{
+ struct exynos_acpm_shmem_chan *shmem_chans, *shmem_chan;
+ struct exynos_acpm_shmem *shmem = exynos_acpm->shmem;
+ struct mbox_chan *mbox_chan, *mbox_chans;
+ struct exynos_acpm_chan *chan, *chans;
+ struct device *dev = exynos_acpm->dev;
+ int i, ret;
+
+ exynos_acpm->num_chans = readl_relaxed(&shmem->num_chans);
+
+ mbox_chans = devm_kcalloc(dev, exynos_acpm->num_chans,
+ sizeof(*mbox_chans), GFP_KERNEL);
+ if (!mbox_chans)
+ return -ENOMEM;
+
+ chans = devm_kcalloc(dev, exynos_acpm->num_chans, sizeof(*chans),
+ GFP_KERNEL);
+ if (!chans)
+ return -ENOMEM;
+
+ shmem_chans = exynos_acpm_get_iomem_addr(exynos_acpm->sram_base,
+ &shmem->chans);
+
+ for (i = 0; i < exynos_acpm->num_chans; i++) {
+ shmem_chan = &shmem_chans[i];
+ mbox_chan = &mbox_chans[i];
+ chan = &chans[i];
+
+ /* Set parameters for the mailbox channel. */
+ mbox_chan->con_priv = chan;
+ mbox_chan->mbox = exynos_acpm->mbox;
+
+ /* Set parameters for the ACPM channel. */
+ chan->mlen = readl_relaxed(&shmem_chan->mlen);
+
+ ret = exynos_acpm_alloc_cmds(exynos_acpm, chan);
+ if (ret)
+ return ret;
+
+ chan->polling = readl_relaxed(&shmem_chan->polling);
+ chan->id = readl_relaxed(&shmem_chan->id);
+ chan->qlen = readl_relaxed(&shmem_chan->qlen);
+
+ exynos_acpm_rx_queue_init(exynos_acpm, shmem_chan, &chan->rx);
+ exynos_acpm_tx_queue_init(exynos_acpm, shmem_chan, &chan->tx);
+
+ mutex_init(&chan->rx_lock);
+ spin_lock_init(&chan->tx_lock);
+
+ dev_vdbg(dev, "ID = %d poll = %d, mlen = %d, qlen = %d\n",
+ chan->id, chan->polling, chan->mlen, chan->qlen);
+ }
+
+ /* Save pointers to the ACPM and mailbox channels. */
+ exynos_acpm->mbox->chans = mbox_chans;
+ exynos_acpm->chans = chans;
+
+ return 0;
+}
+
+static const struct of_device_id exynos_acpm_match[] = {
+ { .compatible = "google,gs101-acpm" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, exynos_acpm_match);
+
+static int exynos_acpm_probe(struct platform_device *pdev)
+{
+ struct device_node *node = pdev->dev.of_node;
+ struct device *dev = &pdev->dev;
+ struct exynos_acpm *exynos_acpm;
+ struct mbox_controller *mbox;
+ struct device_node *shmem;
+ resource_size_t size;
+ struct resource res;
+ const __be32 *prop;
+ int ret;
+
+ exynos_acpm = devm_kzalloc(dev, sizeof(*exynos_acpm), GFP_KERNEL);
+ if (!exynos_acpm)
+ return -ENOMEM;
+
+ mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+ if (!mbox)
+ return -ENOMEM;
+
+ exynos_acpm->regs = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(exynos_acpm->regs))
+ return PTR_ERR(exynos_acpm->regs);
+
+ shmem = of_parse_phandle(node, "shmem", 0);
+ ret = of_address_to_resource(shmem, 0, &res);
+ of_node_put(shmem);
+ if (ret) {
+ dev_err(dev, "Failed to get shared memory.\n");
+ return ret;
+ }
+
+ size = resource_size(&res);
+ exynos_acpm->sram_base = devm_ioremap(dev, res.start, size);
+ if (!exynos_acpm->sram_base) {
+ dev_err(dev, "Failed to ioremap shared memory.\n");
+ return -ENOMEM;
+ }
+
+ prop = of_get_property(node, "initdata-base", NULL);
+ if (!prop) {
+ dev_err(dev, "Parsing initdata_base failed.\n");
+ return -EINVAL;
+ }
+
+ exynos_acpm->pclk = devm_clk_get(dev, "pclk");
+ if (IS_ERR(exynos_acpm->pclk)) {
+ dev_err(dev, "Missing peripheral clock.\n");
+ return PTR_ERR(exynos_acpm->pclk);
+ }
+
+ ret = clk_prepare_enable(exynos_acpm->pclk);
+ if (ret) {
+ dev_err(dev, "Failed to enable the peripheral clock.\n");
+ return ret;
+ }
+
+ exynos_acpm->wq = alloc_workqueue("exynos-acpm-wq", 0, 0);
+ if (!exynos_acpm->wq)
+ return -ENOMEM;
+
+ exynos_acpm->dev = dev;
+ exynos_acpm->mbox = mbox;
+ exynos_acpm->shmem = exynos_acpm->sram_base + be32_to_cpup(prop);
+
+ ret = exynos_acpm_chans_init(exynos_acpm);
+ if (ret)
+ return ret;
+
+ mbox->num_chans = exynos_acpm->num_chans;
+ mbox->dev = dev;
+ mbox->ops = &exynos_acpm_chan_ops;
+
+ platform_set_drvdata(pdev, exynos_acpm);
+
+ /* Mask out all interrupts. We support just polling channels for now. */
+ writel_relaxed(EXYNOS_ACPM_INTMR0_MASK,
+ exynos_acpm->regs + EXYNOS_ACPM_INTMR0);
+
+ ret = devm_mbox_controller_register(dev, mbox);
+ if (ret)
+ dev_err(dev, "Failed to register mbox_controller(%d).\n", ret);
+
+ return ret;
+}
+
+static void exynos_acpm_remove(struct platform_device *pdev)
+{
+ struct exynos_acpm *exynos_acpm = platform_get_drvdata(pdev);
+
+ flush_workqueue(exynos_acpm->wq);
+ destroy_workqueue(exynos_acpm->wq);
+ clk_disable_unprepare(exynos_acpm->pclk);
+}
+
+static struct platform_driver exynos_acpm_driver = {
+ .probe = exynos_acpm_probe,
+ .remove = exynos_acpm_remove,
+ .driver = {
+ .name = "exynos-acpm",
+ .owner = THIS_MODULE,
+ .of_match_table = exynos_acpm_match,
+ },
+};
+module_platform_driver(exynos_acpm_driver);
+
+MODULE_AUTHOR("Tudor Ambarus <tudor.ambarus@linaro.org>");
+MODULE_DESCRIPTION("EXYNOS ACPM mailbox driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mailbox/exynos-acpm-message.h b/include/linux/mailbox/exynos-acpm-message.h
new file mode 100644
index 000000000000..3799868c40b8
--- /dev/null
+++ b/include/linux/mailbox/exynos-acpm-message.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright 2024 Linaro Ltd.
+ */
+
+#ifndef _LINUX_EXYNOS_ACPM_MESSAGE_H_
+#define _LINUX_EXYNOS_ACPM_MESSAGE_H_
+
+#include <linux/types.h>
+
+/**
+ * struct exynos_acpm_message - exynos ACPM mailbox message format.
+ * @cmd: pointer to u32 command.
+ * @len: length of the command.
+ */
+struct exynos_acpm_message {
+ u32 *cmd;
+ size_t len;
+};
+
+#endif /* _LINUX_EXYNOS_ACPM_MESSAGE_H_ */
--
2.47.0.rc1.288.g06298d1525-googHi Tudor
> -----Original Message-----
> From: Tudor Ambarus <tudor.ambarus@linaro.org>
> Sent: Thursday, October 17, 2024 10:07 PM
> To: jassisinghbrar@gmail.com; krzk@kernel.org
> Cc: alim.akhtar@samsung.com; mst@redhat.com; javierm@redhat.com;
> tzimmermann@suse.de; bartosz.golaszewski@linaro.org;
> luzmaximilian@gmail.com; sudeep.holla@arm.com;
> conor.dooley@microchip.com; bjorn@rivosinc.com; ulf.hansson@linaro.org;
> linux-samsung-soc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> arm-kernel@lists.infradead.org; marcan@marcan.st; neal@gompa.dev;
> alyssa@rosenzweig.io; broonie@kernel.org; andre.draszik@linaro.org;
> willmcvicker@google.com; peter.griffin@linaro.org; kernel-
> team@android.com; vincent.guittot@linaro.org; daniel.lezcano@linaro.org;
> Tudor Ambarus <tudor.ambarus@linaro.org>
> Subject: [PATCH v2 2/2] firmware: add exynos acpm driver
>
> 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.
>
> The mailbox channels are initialized based on the configuration data found
at
> a specific offset into the shared memory (mmio-sram). The configuration
> data consists of channel id, message and queue lengths, pointers to the RX
> and TX queues which are also part of the SRAM, and whether RX works by
> polling or interrupts. All known clients of this driver are using polling
channels,
> thus the driver implements for now just polling mode.
>
> Add support for the exynos acpm core driver. Helper drivers will follow.
> These will construct the mailbox messages in the format expected by the
> firmware.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
> drivers/firmware/Kconfig | 1 +
> drivers/firmware/Makefile | 1 +
> drivers/firmware/samsung/Kconfig | 11 +
> drivers/firmware/samsung/Makefile | 3 +
> drivers/firmware/samsung/exynos-acpm.c | 703
> ++++++++++++++++++++
> include/linux/mailbox/exynos-acpm-message.h | 21 +
> 6 files changed, 740 insertions(+)
> create mode 100644 drivers/firmware/samsung/Kconfig create mode
> 100644 drivers/firmware/samsung/Makefile create mode 100644
> drivers/firmware/samsung/exynos-acpm.c
> create mode 100644 include/linux/mailbox/exynos-acpm-message.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..f908773c1441
> --- /dev/null
> +++ b/drivers/firmware/samsung/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config EXYNOS_ACPM
This looks misleading to me, as you mentioned below, ACPM is a FW which runs
on APM module, and
The proposed driver is a communication method between Application processor
and APM module,
Which is via MAILBOX.
So preferably EXYNOS_MAILBOX_APM is more meaningful here.
> + tristate "Exynos ACPM (Alive Clock and Power Manager) driver
> support"
> + select MAILBOX
> + help
> + ACPM 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 module provides the means to communicate with the ACPM
> firmware.
> diff --git a/drivers/firmware/samsung/Makefile
> b/drivers/firmware/samsung/Makefile
> new file mode 100644
> index 000000000000..35ff3076bbea
> --- /dev/null
> +++ b/drivers/firmware/samsung/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_EXYNOS_ACPM) += exynos-acpm.o
> diff --git a/drivers/firmware/samsung/exynos-acpm.c
> b/drivers/firmware/samsung/exynos-acpm.c
> new file mode 100644
> index 000000000000..c3ad4dc7a9e0
> --- /dev/null
> +++ b/drivers/firmware/samsung/exynos-acpm.c
> @@ -0,0 +1,703 @@
> +// 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/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/mailbox/exynos-acpm-message.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define EXYNOS_ACPM_MCUCTRL 0x0 /* Mailbox Control
> Register */
> +#define EXYNOS_ACPM_INTCR0 0x24 /* Interrupt Clear
> Register 0 */
> +#define EXYNOS_ACPM_INTMR0 0x28 /* Interrupt Mask
> Register 0 */
> +#define EXYNOS_ACPM_INTSR0 0x2c /* Interrupt Status
> Register 0 */
> +#define EXYNOS_ACPM_INTMSR0 0x30 /* Interrupt Mask
> Status Register 0 */
> +#define EXYNOS_ACPM_INTGR1 0x40 /* Interrupt
> Generation Register 1 */
> +#define EXYNOS_ACPM_INTMR1 0x48 /* Interrupt Mask
> Register 1 */
> +#define EXYNOS_ACPM_INTSR1 0x4c /* Interrupt Status
> Register 1 */
> +#define EXYNOS_ACPM_INTMSR1 0x50 /* Interrupt Mask
> Status Register 1 */
> +
> +#define EXYNOS_ACPM_INTMR0_MASK GENMASK(15, 0)
> +#define EXYNOS_ACPM_PROTOCOL_SEQNUM GENMASK(21, 16)
> +
> +/* The unit of counter is 20 us. 5000 * 20 = 100 ms */
> +#define EXYNOS_ACPM_POLL_TIMEOUT 5000
> +#define EXYNOS_ACPM_TX_TIMEOUT_US 500000
> +
> +/**
> + * struct exynos_acpm_shmem - mailbox shared memory configuration
> information.
> + * @reserved: reserved for future use.
> + * @chans: offset to array of struct exynos_acpm_shmem_chan.
> + * @reserved1: reserved for future use.
> + * @num_chans: number of channels.
> + */
> +struct exynos_acpm_shmem {
> + u32 reserved[2];
> + u32 chans;
> + u32 reserved1[3];
> + u32 num_chans;
> +};
> +
> +/**
> + * struct exynos_acpm_shmem_chan - descriptor of a shared memory
> channel.
> + *
> + * @id: channel ID.
> + * @reserved: reserved for future use.
> + * @rx_rear: rear pointer of RX queue.
> + * @rx_front: front pointer of RX queue.
> + * @rx_base: base address of RX queue.
> + * @reserved1: reserved for future use.
> + * @tx_rear: rear pointer of TX queue.
> + * @tx_front: front pointer of TX queue.
> + * @tx_base: base address of TX queue.
> + * @qlen: queue length. Applies to both TX/RX queues.
> + * @mlen: message length. Applies to both TX/RX queues.
> + * @reserved2: reserved for future use.
> + * @polling: true when the channel works on polling.
> + */
> +struct exynos_acpm_shmem_chan {
> + 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 polling;
> +};
> +
> +/**
> + * struct exynos_acpm_queue - exynos acpm queue.
> + *
> + * @rear: rear address of the queue.
> + * @front: front address of the queue.
> + * @base: base address of the queue.
> + */
> +struct exynos_acpm_queue {
> + void __iomem *rear;
> + void __iomem *front;
> + void __iomem *base;
> +};
> +
> +/**
> + * struct exynos_acpm_rx_data - RX queue data.
> + *
> + * @cmd: pointer to where the data shall be saved.
> + * @response: true if the client expects the RX data.
> + */
> +struct exynos_acpm_rx_data {
> + u32 *cmd;
> + bool response;
> +};
> +
> +#define EXYNOS_ACPM_SEQNUM_MAX 64
> +
> +/**
> + * struct exynos_acpm_chan - driver internal representation of a channel.
> + * @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.
> + * @rx_lock: protects RX queue. The RX queue is accessed just in
> + * process context.
> + * @tx_lock: protects TX 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.
> + * @polling: true when the channel works on polling.
> + * @bitmap_seqnum: bitmap that tracks the messages on the TX/RX
> queues.
> + * @rx_data: internal buffer used to drain the RX queue.
> + */
> +struct exynos_acpm_chan {
> + struct exynos_acpm_queue tx;
> + struct exynos_acpm_queue rx;
> + struct mutex rx_lock;
> + spinlock_t tx_lock;
> +
> + unsigned int qlen;
> + unsigned int mlen;
> + u8 seqnum;
> + u8 id;
> + bool polling;
> +
> + DECLARE_BITMAP(bitmap_seqnum, EXYNOS_ACPM_SEQNUM_MAX
> - 1);
> + struct exynos_acpm_rx_data
> rx_data[EXYNOS_ACPM_SEQNUM_MAX]; };
> +
> +/**
> + * struct exynos_acpm - driver's private data.
> + * @shmem: pointer to the SRAM configuration data.
> + * @chans: pointer to the ACPM channel parameters retrieved from
> SRAM.
> + * @sram_base: base address of SRAM.
> + * @regs: mailbox registers base address.
> + * @mbox: pointer to the mailbox controller.
> + * @wq: pointer to workqueue.
> + * @dev: pointer to the exynos-acpm device.
> + * @pclk: pointer to the mailbox peripheral clock.
> + * @num_chans: number of channels available for this controller.
> + */
> +struct exynos_acpm {
> + struct exynos_acpm_shmem *shmem;
> + struct exynos_acpm_chan *chans;
> + void __iomem *sram_base;
> + void __iomem *regs;
> + struct mbox_controller *mbox;
> + struct workqueue_struct *wq;
> + struct device *dev;
> + struct clk *pclk;
> + u32 num_chans;
> +};
> +
> +/**
> + * struct exynos_acpm_work_data - data structure representing the work.
> + * @mbox_chan: pointer to the mailbox channel.
> + * @req: pointer to the mailbox request.
> + * @callback: pointer to a callback function to be invoked upon
> + * completion of this request.
> + * @work: describes the task to be executed.
> + */
> +struct exynos_acpm_work_data {
> + struct mbox_chan *mbox_chan;
> + struct mbox_request *req;
> + void (*callback)(struct exynos_acpm_work_data *work_data, int
> status);
> + struct work_struct work;
> +};
> +
> +static int exynos_acpm_get_rx(struct mbox_chan *mbox_chan,
> + struct mbox_request *req)
> +{
> + struct exynos_acpm_chan *chan = mbox_chan->con_priv;
> + struct exynos_acpm_message *tx = req->tx;
> + struct exynos_acpm_message *rx = req->rx;
> + struct exynos_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(chan->rx.front);
> + i = readl_relaxed(chan->rx.rear);
> +
> + /* Bail out if RX is empty. */
> + if (i == rx_front)
> + return 0;
> +
> + base = chan->rx.base;
> + mlen = chan->mlen;
> +
> + tx_seqnum = FIELD_GET(EXYNOS_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(EXYNOS_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 = &chan->rx_data[seqnum];
> +
> + if (rx_data->response) {
> + if (rx_seqnum == tx_seqnum) {
> + __ioread32_copy(rx->cmd, addr, req->rxlen /
> 4);
> + rx_set = true;
> + clear_bit(seqnum, chan->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,
> + req->rxlen / 4);
> + }
> + } else {
> + clear_bit(seqnum, chan->bitmap_seqnum);
> + }
> +
> + i = (i + 1) % chan->qlen;
> + } while (i != rx_front);
> +
> + /* We saved all responses, mark RX empty. */
> + writel_relaxed(rx_front, chan->rx.rear);
> +
> + /* Flush SRAM posted writes. */
> + readl_relaxed(chan->rx.front);
> +
> + /*
> + * If the response was not in this iteration of the queue, check if
the
> + * RX data was previously saved.
> + */
> + rx_data = &chan->rx_data[tx_seqnum - 1];
> + if (!rx_set && rx_data->response) {
> + rx_seqnum =
> FIELD_GET(EXYNOS_ACPM_PROTOCOL_SEQNUM,
> + rx_data->cmd[0]);
> +
> + if (rx_seqnum == tx_seqnum) {
> + memcpy(rx->cmd, rx_data->cmd, req->rxlen);
> + clear_bit(rx_seqnum - 1, chan->bitmap_seqnum);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int exynos_acpm_dequeue_by_polling(struct mbox_chan
> *mbox_chan,
> + struct mbox_request *req)
> +{
> + struct exynos_acpm_chan *chan = mbox_chan->con_priv;
> + struct exynos_acpm_message *tx = req->tx;
> + struct device *dev = mbox_chan->mbox->dev;
> + unsigned int cnt_20us = 0;
> + u32 seqnum;
> + int ret;
> +
> + seqnum = FIELD_GET(EXYNOS_ACPM_PROTOCOL_SEQNUM, tx-
> >cmd[0]);
> +
> + do {
> + ret = mutex_lock_interruptible(&chan->rx_lock);
> + if (ret)
> + return ret;
> + ret = exynos_acpm_get_rx(mbox_chan, req);
> + mutex_unlock(&chan->rx_lock);
> + if (ret)
> + return ret;
> +
> + if (!test_bit(seqnum - 1, chan->bitmap_seqnum)) {
> + dev_vdbg(dev, "cnt_20us = %d.\n", cnt_20us);
> + return 0;
> + }
> +
> + /* Determined experimentally. */
> + usleep_range(20, 30);
> + cnt_20us++;
> + } while (cnt_20us < EXYNOS_ACPM_POLL_TIMEOUT);
> +
> + dev_err(dev, "Timeout! ch:%u s:%u bitmap:%lx, cnt_20us = %d.\n",
> + chan->id, seqnum, chan->bitmap_seqnum[0], cnt_20us);
> +
> + return -ETIME;
> +}
> +
> +static void exynos_acpm_done(struct exynos_acpm_work_data
> *work_data,
> +int status) {
> + struct mbox_request *req = work_data->req;
> +
> + kfree(work_data);
> + mbox_request_complete(req, status);
> +}
> +
> +static void exynos_acpm_work_handler(struct work_struct *work) {
> + struct exynos_acpm_work_data *work_data =
> + container_of(work, struct exynos_acpm_work_data, work);
> + struct mbox_chan *mbox_chan = work_data->mbox_chan;
> + int ret;
> +
> + ret = exynos_acpm_dequeue_by_polling(mbox_chan, work_data-
> >req);
> + work_data->callback(work_data, ret);
> +}
> +
> +static struct exynos_acpm_work_data *
> + exynos_acpm_init_work(struct mbox_chan *mbox_chan,
> + struct mbox_request *req)
> +{
> + struct exynos_acpm_work_data *work_data;
> + gfp_t gfp = (req->flags & MBOX_REQ_MAY_SLEEP) ? GFP_KERNEL :
> +GFP_ATOMIC;
> +
> + work_data = kmalloc(sizeof(*work_data), gfp);
> + if (!work_data)
> + return ERR_PTR(-ENOMEM);
> +
> + work_data->mbox_chan = mbox_chan;
> + work_data->req = req;
> + work_data->callback = exynos_acpm_done;
> + INIT_WORK(&work_data->work, exynos_acpm_work_handler);
> +
> + return work_data;
> +}
> +
> +static void exynos_acpm_prepare_request(struct mbox_chan *mbox_chan,
> + struct mbox_request *req)
> +{
> + struct exynos_acpm_chan *chan = mbox_chan->con_priv;
> + struct exynos_acpm_message *tx = req->tx;
> + struct exynos_acpm_rx_data *rx_data;
> +
> + /* Prevent chan->seqnum from being re-used */
> + do {
> + if (++chan->seqnum == EXYNOS_ACPM_SEQNUM_MAX)
> + chan->seqnum = 1;
> + } while (test_bit(chan->seqnum - 1, chan->bitmap_seqnum));
> +
> + tx->cmd[0] |= FIELD_PREP(EXYNOS_ACPM_PROTOCOL_SEQNUM,
> chan->seqnum);
> +
> + /* Clear data for upcoming responses */
> + rx_data = &chan->rx_data[chan->seqnum - 1];
> + memset(rx_data->cmd, 0, sizeof(*(rx_data->cmd)) * chan->mlen);
> + if (req->rx)
> + rx_data->response = true;
> +
> + /* Flag the index based on seqnum. (seqnum: 1~63, bitmap: 0~62) */
> + set_bit(chan->seqnum - 1, chan->bitmap_seqnum); }
> +
> +static int exynos_acpm_wait_for_queue_slots(struct mbox_chan
> *mbox_chan,
> + u32 next_tx_front)
> +{
> + struct exynos_acpm_chan *chan = mbox_chan->con_priv;
> + struct device *dev = mbox_chan->mbox->dev;
> + 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(chan->rx.front, val,
> + next_tx_front != val, 1,
> +
> EXYNOS_ACPM_TX_TIMEOUT_US);
> + if (ret) {
> + dev_err(dev, "RX front can not keep up with TX front.\n");
> + return ret;
> + }
> +
> + ret = readl_relaxed_poll_timeout_atomic(chan->tx.rear, val,
> + next_tx_front != val, 1,
> +
> EXYNOS_ACPM_TX_TIMEOUT_US);
> + if (ret)
> + dev_err(dev, "TX queue is full.\n");
> +
> + return ret;
> +}
> +
> +static int exynos_acpm_send_request(struct mbox_chan *mbox_chan,
> + struct mbox_request *req)
> +{
> + struct exynos_acpm *exynos_acpm = dev_get_drvdata(mbox_chan-
> >mbox->dev);
> + struct exynos_acpm_chan *chan = mbox_chan->con_priv;
> + struct exynos_acpm_message *tx = req->tx;
> + struct exynos_acpm_work_data *work_data;
> + u32 idx, tx_front;
> + unsigned long flags;
> + int ret;
> +
> + if (!tx || !tx->cmd || req->txlen > chan->mlen ||
> + req->rxlen > chan->mlen)
> + return -EINVAL;
> +
> + work_data = exynos_acpm_init_work(mbox_chan, req);
> + if (IS_ERR(work_data))
> + return PTR_ERR(work_data);
> +
> + spin_lock_irqsave(&chan->tx_lock, flags);
> +
> + tx_front = readl_relaxed(chan->tx.front);
> + idx = (tx_front + 1) % chan->qlen;
> +
> + ret = exynos_acpm_wait_for_queue_slots(mbox_chan, idx);
> + if (ret)
> + goto exit;
> +
> + exynos_acpm_prepare_request(mbox_chan, req);
> +
> + /* Write TX command. */
> + __iowrite32_copy(chan->tx.base + chan->mlen * tx_front, tx->cmd,
> + req->txlen / 4);
> +
> + /* Advance TX front. */
> + writel_relaxed(idx, chan->tx.front);
> +
> + /* Flush SRAM posted writes. */
> + readl_relaxed(chan->tx.front);
> +
> + /* Generate ACPM interrupt. */
> + writel_relaxed(BIT(chan->id), exynos_acpm->regs +
> EXYNOS_ACPM_INTGR1);
> +
> + /* Flush mailbox controller posted writes. */
> + readl_relaxed(exynos_acpm->regs + EXYNOS_ACPM_MCUCTRL);
> +
> + spin_unlock_irqrestore(&chan->tx_lock, flags);
> +
> + queue_work(exynos_acpm->wq, &work_data->work);
> +
> + return -EINPROGRESS;
> +exit:
> + spin_unlock_irqrestore(&chan->tx_lock, flags);
> + kfree(work_data);
> + return ret;
> +}
> +
> +static int exynos_acpm_chan_startup(struct mbox_chan *mbox_chan) {
> + struct exynos_acpm_chan *chan = mbox_chan->con_priv;
> +
> + if (!chan->polling) {
> + dev_err(mbox_chan->mbox->dev, "IRQs not
> supported.\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static const struct mbox_chan_ops exynos_acpm_chan_ops = {
> + .send_request = exynos_acpm_send_request,
> + .startup = exynos_acpm_chan_startup,
> +};
> +
> +static void __iomem *exynos_acpm_get_iomem_addr(void __iomem
> *base,
> + void __iomem *addr)
> +{
> + u32 offset;
> +
> + offset = readl_relaxed(addr);
> + return base + offset;
> +}
> +
> +static void exynos_acpm_rx_queue_init(struct exynos_acpm
> *exynos_acpm,
> + struct exynos_acpm_shmem_chan
> *shmem_chan,
> + struct exynos_acpm_queue *rx) {
> + void __iomem *base = exynos_acpm->sram_base;
> +
> + rx->base = exynos_acpm_get_iomem_addr(base, &shmem_chan-
> >tx_base);
> + rx->front = exynos_acpm_get_iomem_addr(base, &shmem_chan-
> >tx_front);
> + rx->rear = exynos_acpm_get_iomem_addr(base, &shmem_chan-
> >tx_rear); }
> +
> +static void exynos_acpm_tx_queue_init(struct exynos_acpm
> *exynos_acpm,
> + struct exynos_acpm_shmem_chan
> *shmem_chan,
> + struct exynos_acpm_queue *tx) {
> + void __iomem *base = exynos_acpm->sram_base;
> +
> + tx->base = exynos_acpm_get_iomem_addr(base, &shmem_chan-
> >rx_base);
> + tx->front = exynos_acpm_get_iomem_addr(base, &shmem_chan-
> >rx_front);
> + tx->rear = exynos_acpm_get_iomem_addr(base, &shmem_chan-
> >rx_rear); }
> +
> +static int exynos_acpm_alloc_cmds(struct exynos_acpm *exynos_acpm,
> + struct exynos_acpm_chan *chan)
> +{
> + struct device *dev = exynos_acpm->dev;
> + struct exynos_acpm_rx_data *rx_data;
> + unsigned int mlen = chan->mlen;
> + int i;
> +
> + for (i = 0; i < EXYNOS_ACPM_SEQNUM_MAX; i++) {
> + rx_data = &chan->rx_data[i];
> + rx_data->cmd = devm_kcalloc(dev, mlen, sizeof(*(rx_data-
> >cmd)),
> + GFP_KERNEL);
> + if (!rx_data->cmd)
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static int exynos_acpm_chans_init(struct exynos_acpm *exynos_acpm) {
> + struct exynos_acpm_shmem_chan *shmem_chans, *shmem_chan;
> + struct exynos_acpm_shmem *shmem = exynos_acpm->shmem;
> + struct mbox_chan *mbox_chan, *mbox_chans;
> + struct exynos_acpm_chan *chan, *chans;
> + struct device *dev = exynos_acpm->dev;
> + int i, ret;
> +
> + exynos_acpm->num_chans = readl_relaxed(&shmem->num_chans);
> +
> + mbox_chans = devm_kcalloc(dev, exynos_acpm->num_chans,
> + sizeof(*mbox_chans), GFP_KERNEL);
> + if (!mbox_chans)
> + return -ENOMEM;
> +
> + chans = devm_kcalloc(dev, exynos_acpm->num_chans,
> sizeof(*chans),
> + GFP_KERNEL);
> + if (!chans)
> + return -ENOMEM;
> +
> + shmem_chans = exynos_acpm_get_iomem_addr(exynos_acpm-
> >sram_base,
> + &shmem->chans);
> +
> + for (i = 0; i < exynos_acpm->num_chans; i++) {
> + shmem_chan = &shmem_chans[i];
> + mbox_chan = &mbox_chans[i];
> + chan = &chans[i];
> +
> + /* Set parameters for the mailbox channel. */
> + mbox_chan->con_priv = chan;
> + mbox_chan->mbox = exynos_acpm->mbox;
> +
> + /* Set parameters for the ACPM channel. */
> + chan->mlen = readl_relaxed(&shmem_chan->mlen);
> +
> + ret = exynos_acpm_alloc_cmds(exynos_acpm, chan);
> + if (ret)
> + return ret;
> +
> + chan->polling = readl_relaxed(&shmem_chan->polling);
> + chan->id = readl_relaxed(&shmem_chan->id);
> + chan->qlen = readl_relaxed(&shmem_chan->qlen);
> +
> + exynos_acpm_rx_queue_init(exynos_acpm, shmem_chan,
> &chan->rx);
> + exynos_acpm_tx_queue_init(exynos_acpm, shmem_chan,
> &chan->tx);
> +
> + mutex_init(&chan->rx_lock);
> + spin_lock_init(&chan->tx_lock);
> +
> + dev_vdbg(dev, "ID = %d poll = %d, mlen = %d, qlen = %d\n",
> + chan->id, chan->polling, chan->mlen, chan->qlen);
> + }
> +
> + /* Save pointers to the ACPM and mailbox channels. */
> + exynos_acpm->mbox->chans = mbox_chans;
> + exynos_acpm->chans = chans;
> +
> + return 0;
> +}
> +
> +static const struct of_device_id exynos_acpm_match[] = {
> + { .compatible = "google,gs101-acpm" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, exynos_acpm_match);
> +
> +static int exynos_acpm_probe(struct platform_device *pdev) {
> + struct device_node *node = pdev->dev.of_node;
> + struct device *dev = &pdev->dev;
> + struct exynos_acpm *exynos_acpm;
> + struct mbox_controller *mbox;
> + struct device_node *shmem;
> + resource_size_t size;
> + struct resource res;
> + const __be32 *prop;
> + int ret;
> +
> + exynos_acpm = devm_kzalloc(dev, sizeof(*exynos_acpm),
> GFP_KERNEL);
> + if (!exynos_acpm)
> + return -ENOMEM;
> +
> + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> + if (!mbox)
> + return -ENOMEM;
> +
> + exynos_acpm->regs = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(exynos_acpm->regs))
> + return PTR_ERR(exynos_acpm->regs);
> +
> + shmem = of_parse_phandle(node, "shmem", 0);
> + ret = of_address_to_resource(shmem, 0, &res);
> + of_node_put(shmem);
> + if (ret) {
> + dev_err(dev, "Failed to get shared memory.\n");
> + return ret;
> + }
> +
> + size = resource_size(&res);
> + exynos_acpm->sram_base = devm_ioremap(dev, res.start, size);
> + if (!exynos_acpm->sram_base) {
> + dev_err(dev, "Failed to ioremap shared memory.\n");
> + return -ENOMEM;
> + }
> +
> + prop = of_get_property(node, "initdata-base", NULL);
> + if (!prop) {
> + dev_err(dev, "Parsing initdata_base failed.\n");
> + return -EINVAL;
> + }
> +
> + exynos_acpm->pclk = devm_clk_get(dev, "pclk");
> + if (IS_ERR(exynos_acpm->pclk)) {
> + dev_err(dev, "Missing peripheral clock.\n");
> + return PTR_ERR(exynos_acpm->pclk);
> + }
> +
> + ret = clk_prepare_enable(exynos_acpm->pclk);
> + if (ret) {
> + dev_err(dev, "Failed to enable the peripheral clock.\n");
> + return ret;
> + }
> +
> + exynos_acpm->wq = alloc_workqueue("exynos-acpm-wq", 0, 0);
> + if (!exynos_acpm->wq)
> + return -ENOMEM;
> +
> + exynos_acpm->dev = dev;
> + exynos_acpm->mbox = mbox;
> + exynos_acpm->shmem = exynos_acpm->sram_base +
> be32_to_cpup(prop);
> +
> + ret = exynos_acpm_chans_init(exynos_acpm);
> + if (ret)
> + return ret;
> +
> + mbox->num_chans = exynos_acpm->num_chans;
> + mbox->dev = dev;
> + mbox->ops = &exynos_acpm_chan_ops;
> +
> + platform_set_drvdata(pdev, exynos_acpm);
> +
> + /* Mask out all interrupts. We support just polling channels for
now.
> */
> + writel_relaxed(EXYNOS_ACPM_INTMR0_MASK,
> + exynos_acpm->regs + EXYNOS_ACPM_INTMR0);
> +
> + ret = devm_mbox_controller_register(dev, mbox);
> + if (ret)
> + dev_err(dev, "Failed to register mbox_controller(%d).\n",
> ret);
> +
> + return ret;
> +}
> +
> +static void exynos_acpm_remove(struct platform_device *pdev) {
> + struct exynos_acpm *exynos_acpm = platform_get_drvdata(pdev);
> +
> + flush_workqueue(exynos_acpm->wq);
> + destroy_workqueue(exynos_acpm->wq);
> + clk_disable_unprepare(exynos_acpm->pclk);
> +}
> +
> +static struct platform_driver exynos_acpm_driver = {
> + .probe = exynos_acpm_probe,
> + .remove = exynos_acpm_remove,
> + .driver = {
> + .name = "exynos-acpm",
> + .owner = THIS_MODULE,
> + .of_match_table = exynos_acpm_match,
> + },
> +};
> +module_platform_driver(exynos_acpm_driver);
> +
> +MODULE_AUTHOR("Tudor Ambarus <tudor.ambarus@linaro.org>");
> +MODULE_DESCRIPTION("EXYNOS ACPM mailbox driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mailbox/exynos-acpm-message.h
> b/include/linux/mailbox/exynos-acpm-message.h
> new file mode 100644
> index 000000000000..3799868c40b8
> --- /dev/null
> +++ b/include/linux/mailbox/exynos-acpm-message.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright 2024 Linaro Ltd.
> + */
> +
> +#ifndef _LINUX_EXYNOS_ACPM_MESSAGE_H_
> +#define _LINUX_EXYNOS_ACPM_MESSAGE_H_
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct exynos_acpm_message - exynos ACPM mailbox message format.
> + * @cmd: pointer to u32 command.
> + * @len: length of the command.
> + */
> +struct exynos_acpm_message {
> + u32 *cmd;
> + size_t len;
> +};
> +
> +#endif /* _LINUX_EXYNOS_ACPM_MESSAGE_H_ */
> --
> 2.47.0.rc1.288.g06298d1525-goog
On 10/21/24 5:47 PM, Alim Akhtar wrote: > Hi Tudor Hi, Alim! Thanks for the review! >> diff --git a/drivers/firmware/samsung/Kconfig >> b/drivers/firmware/samsung/Kconfig >> new file mode 100644 >> index 000000000000..f908773c1441 >> --- /dev/null >> +++ b/drivers/firmware/samsung/Kconfig >> @@ -0,0 +1,11 @@ >> +# SPDX-License-Identifier: GPL-2.0-only >> + >> +config EXYNOS_ACPM > > This looks misleading to me, as you mentioned below, ACPM is a FW which runs > on APM module, and > The proposed driver is a communication method between Application processor > and APM module, > Which is via MAILBOX. > So preferably EXYNOS_MAILBOX_APM is more meaningful here. This seems more accurate indeed. The design document that I have refers to the protocol as "ACPM IPC", so maybe I shall stick with EXYNOS_ACPM_IPC. I'll also need to prefix all the definitions in the driver with exynos_acpm_ipc_* which will result in long names. But I guess there's nothing we can do about it. Cheers, ta
On 10/17/24 5:36 PM, Tudor Ambarus wrote:
> +static int exynos_acpm_chans_init(struct exynos_acpm *exynos_acpm)
> +{
> + struct exynos_acpm_shmem_chan *shmem_chans, *shmem_chan;
> + struct exynos_acpm_shmem *shmem = exynos_acpm->shmem;
As Krzysztof has already noticed, I need to use the __iomem pointer
token when referring to shmem structures. This could be caught with
sparse as well, lesson learnt:
drivers/firmware/samsung/exynos-acpm.c:493:54: sparse: sparse:
incorrect type in argument 2 (different address spaces) @@ expected
void [noderef] __iomem *addr @@ got unsigned int * @@
The following changes will be needed for v3:
--- a/drivers/firmware/samsung/exynos-acpm.c
+++ b/drivers/firmware/samsung/exynos-acpm.c
@@ -161,7 +161,7 @@ struct exynos_acpm_chan {
* @num_chans: number of channels available for this controller.
*/
struct exynos_acpm {
- struct exynos_acpm_shmem *shmem;
+ struct exynos_acpm_shmem __iomem *shmem;
struct exynos_acpm_chan *chans;
void __iomem *sram_base;
void __iomem *regs;
@@ -485,8 +485,8 @@ static void __iomem *exynos_acpm_get_iomem_addr(void
__iomem *base,
}
static void exynos_acpm_rx_queue_init(struct exynos_acpm *exynos_acpm,
- struct exynos_acpm_shmem_chan
*shmem_chan,
- struct exynos_acpm_queue *rx)
+ struct exynos_acpm_shmem_chan __iomem *shmem_chan,
+ struct exynos_acpm_queue *rx)
{
void __iomem *base = exynos_acpm->sram_base;
@@ -496,8 +496,8 @@ static void exynos_acpm_rx_queue_init(struct
exynos_acpm *exynos_acpm,
}
static void exynos_acpm_tx_queue_init(struct exynos_acpm *exynos_acpm,
- struct exynos_acpm_shmem_chan
*shmem_chan,
- struct exynos_acpm_queue *tx)
+ struct exynos_acpm_shmem_chan __iomem *shmem_chan,
+ struct exynos_acpm_queue *tx)
{
void __iomem *base = exynos_acpm->sram_base;
@@ -527,8 +527,8 @@ static int exynos_acpm_alloc_cmds(struct exynos_acpm
*exynos_acpm,
static int exynos_acpm_chans_init(struct exynos_acpm *exynos_acpm)
{
- struct exynos_acpm_shmem_chan *shmem_chans, *shmem_chan;
- struct exynos_acpm_shmem *shmem = exynos_acpm->shmem;
+ struct exynos_acpm_shmem_chan __iomem *shmem_chans, *shmem_chan;
+ struct exynos_acpm_shmem __iomem *shmem = exynos_acpm->shmem;
struct mbox_chan *mbox_chan, *mbox_chans;
struct exynos_acpm_chan *chan, *chans;
struct device *dev = exynos_acpm->dev;
On 17/10/2024 18:36, Tudor Ambarus wrote:
> 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.
>
> The mailbox channels are initialized based on the configuration data
> found at a specific offset into the shared memory (mmio-sram). The
> configuration data consists of channel id, message and queue lengths,
> pointers to the RX and TX queues which are also part of the SRAM, and
> whether RX works by polling or interrupts. All known clients of this
> driver are using polling channels, thus the driver implements for now
> just polling mode.
>
> Add support for the exynos acpm core driver. Helper drivers will follow.
> These will construct the mailbox messages in the format expected by the
> firmware.
I skimmed through the driver and I do not understand why this is
firmware. You are implementing a mailbox provider/controller.
I did not perform full review yet, just skimmed over the code. I will
take a look a bit later.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
> drivers/firmware/Kconfig | 1 +
> drivers/firmware/Makefile | 1 +
> drivers/firmware/samsung/Kconfig | 11 +
> drivers/firmware/samsung/Makefile | 3 +
> drivers/firmware/samsung/exynos-acpm.c | 703 ++++++++++++++++++++
Please add directory to the Samsung Exynos SoC maintainer entry. I also
encourage adding separate entry for the driver where you would be listed
as maintainer.
There is no firmware tree, so this will be going via Samsung SoC.
> include/linux/mailbox/exynos-acpm-message.h | 21 +
> 6 files changed, 740 insertions(+)
> create mode 100644 drivers/firmware/samsung/Kconfig
> create mode 100644 drivers/firmware/samsung/Makefile
> create mode 100644 drivers/firmware/samsung/exynos-acpm.c
> create mode 100644 include/linux/mailbox/exynos-acpm-message.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..f908773c1441
> --- /dev/null
> +++ b/drivers/firmware/samsung/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config EXYNOS_ACPM
> + tristate "Exynos ACPM (Alive Clock and Power Manager) driver support"
depends ARCH_EXYNOS || COMPILE_TEST
Please also send a arm64 defconfig change making it a module.
> + select MAILBOX
> + help
> + ACPM 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 module provides the means to communicate with the ACPM firmware.
> diff --git a/drivers/firmware/samsung/Makefile b/drivers/firmware/samsung/Makefile
> new file mode 100644
> index 000000000000..35ff3076bbea
> --- /dev/null
> +++ b/drivers/firmware/samsung/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_EXYNOS_ACPM) += exynos-acpm.o
> diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
> new file mode 100644
> index 000000000000..c3ad4dc7a9e0
> --- /dev/null
> +++ b/drivers/firmware/samsung/exynos-acpm.c
> @@ -0,0 +1,703 @@
> +// 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/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/mailbox/exynos-acpm-message.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define EXYNOS_ACPM_MCUCTRL 0x0 /* Mailbox Control Register */
> +#define EXYNOS_ACPM_INTCR0 0x24 /* Interrupt Clear Register 0 */
> +#define EXYNOS_ACPM_INTMR0 0x28 /* Interrupt Mask Register 0 */
> +#define EXYNOS_ACPM_INTSR0 0x2c /* Interrupt Status Register 0 */
> +#define EXYNOS_ACPM_INTMSR0 0x30 /* Interrupt Mask Status Register 0 */
> +#define EXYNOS_ACPM_INTGR1 0x40 /* Interrupt Generation Register 1 */
> +#define EXYNOS_ACPM_INTMR1 0x48 /* Interrupt Mask Register 1 */
> +#define EXYNOS_ACPM_INTSR1 0x4c /* Interrupt Status Register 1 */
> +#define EXYNOS_ACPM_INTMSR1 0x50 /* Interrupt Mask Status Register 1 */
> +
> +#define EXYNOS_ACPM_INTMR0_MASK GENMASK(15, 0)
> +#define EXYNOS_ACPM_PROTOCOL_SEQNUM GENMASK(21, 16)
> +
> +/* The unit of counter is 20 us. 5000 * 20 = 100 ms */
> +#define EXYNOS_ACPM_POLL_TIMEOUT 5000
> +#define EXYNOS_ACPM_TX_TIMEOUT_US 500000
> +
> +/**
> + * struct exynos_acpm_shmem - mailbox shared memory configuration information.
> + * @reserved: reserved for future use.
> + * @chans: offset to array of struct exynos_acpm_shmem_chan.
> + * @reserved1: reserved for future use.
> + * @num_chans: number of channels.
> + */
> +struct exynos_acpm_shmem {
> + u32 reserved[2];
> + u32 chans;
> + u32 reserved1[3];
> + u32 num_chans;
> +};
> +
> +/**
> + * struct exynos_acpm_shmem_chan - descriptor of a shared memory channel.
> + *
> + * @id: channel ID.
> + * @reserved: reserved for future use.
> + * @rx_rear: rear pointer of RX queue.
> + * @rx_front: front pointer of RX queue.
> + * @rx_base: base address of RX queue.
> + * @reserved1: reserved for future use.
> + * @tx_rear: rear pointer of TX queue.
> + * @tx_front: front pointer of TX queue.
> + * @tx_base: base address of TX queue.
> + * @qlen: queue length. Applies to both TX/RX queues.
> + * @mlen: message length. Applies to both TX/RX queues.
> + * @reserved2: reserved for future use.
> + * @polling: true when the channel works on polling.
> + */
> +struct exynos_acpm_shmem_chan {
> + 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 polling;
Why are you storing addresses as u32? Shouldn't these be __iomem*?
I also cannot find any piece of code setting several of above, e.g. tx_base
> +};
> +
> +/**
> + * struct exynos_acpm_queue - exynos acpm queue.
> + *
> + * @rear: rear address of the queue.
> + * @front: front address of the queue.
> + * @base: base address of the queue.
> + */
> +struct exynos_acpm_queue {
> + void __iomem *rear;
> + void __iomem *front;
> + void __iomem *base;
> +};
> +
> +/**
> + * struct exynos_acpm_rx_data - RX queue data.
> + *
> + * @cmd: pointer to where the data shall be saved.
> + * @response: true if the client expects the RX data.
> + */
> +struct exynos_acpm_rx_data {
> + u32 *cmd;
> + bool response;
> +};
> +
> +#define EXYNOS_ACPM_SEQNUM_MAX 64
> +
...
> + if (IS_ERR(work_data))
> + return PTR_ERR(work_data);
> +
> + spin_lock_irqsave(&chan->tx_lock, flags);
> +
> + tx_front = readl_relaxed(chan->tx.front);
> + idx = (tx_front + 1) % chan->qlen;
> +
> + ret = exynos_acpm_wait_for_queue_slots(mbox_chan, idx);
> + if (ret)
> + goto exit;
> +
> + exynos_acpm_prepare_request(mbox_chan, req);
> +
> + /* Write TX command. */
> + __iowrite32_copy(chan->tx.base + chan->mlen * tx_front, tx->cmd,
> + req->txlen / 4);
> +
> + /* Advance TX front. */
> + writel_relaxed(idx, chan->tx.front);
> +
> + /* Flush SRAM posted writes. */
> + readl_relaxed(chan->tx.front);
> +
How does this flush work? Maybe you just miss here barries (s/relaxed//)?
> + /* Generate ACPM interrupt. */
> + writel_relaxed(BIT(chan->id), exynos_acpm->regs + EXYNOS_ACPM_INTGR1);
> +
> + /* Flush mailbox controller posted writes. */
> + readl_relaxed(exynos_acpm->regs + EXYNOS_ACPM_MCUCTRL);
> +
> + spin_unlock_irqrestore(&chan->tx_lock, flags);
> +
> + queue_work(exynos_acpm->wq, &work_data->work);
> +
> + return -EINPROGRESS;
> +exit:
> + spin_unlock_irqrestore(&chan->tx_lock, flags);
> + kfree(work_data);
> + return ret;
> +}
> +
> +static int exynos_acpm_chan_startup(struct mbox_chan *mbox_chan)
> +{
> + struct exynos_acpm_chan *chan = mbox_chan->con_priv;
> +
> + if (!chan->polling) {
> + dev_err(mbox_chan->mbox->dev, "IRQs not supported.\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static const struct mbox_chan_ops exynos_acpm_chan_ops = {
> + .send_request = exynos_acpm_send_request,
> + .startup = exynos_acpm_chan_startup,
> +};
> +
> +static void __iomem *exynos_acpm_get_iomem_addr(void __iomem *base,
> + void __iomem *addr)
> +{
> + u32 offset;
> +
> + offset = readl_relaxed(addr);
> + return base + offset;
> +}
> +
> +static void exynos_acpm_rx_queue_init(struct exynos_acpm *exynos_acpm,
> + struct exynos_acpm_shmem_chan *shmem_chan,
> + struct exynos_acpm_queue *rx)
> +{
> + void __iomem *base = exynos_acpm->sram_base;
> +
> + rx->base = exynos_acpm_get_iomem_addr(base, &shmem_chan->tx_base);
> + rx->front = exynos_acpm_get_iomem_addr(base, &shmem_chan->tx_front);
> + rx->rear = exynos_acpm_get_iomem_addr(base, &shmem_chan->tx_rear);
> +}
> +
> +static void exynos_acpm_tx_queue_init(struct exynos_acpm *exynos_acpm,
> + struct exynos_acpm_shmem_chan *shmem_chan,
> + struct exynos_acpm_queue *tx)
> +{
> + void __iomem *base = exynos_acpm->sram_base;
> +
> + tx->base = exynos_acpm_get_iomem_addr(base, &shmem_chan->rx_base);
> + tx->front = exynos_acpm_get_iomem_addr(base, &shmem_chan->rx_front);
> + tx->rear = exynos_acpm_get_iomem_addr(base, &shmem_chan->rx_rear);
> +}
> +
> +static int exynos_acpm_alloc_cmds(struct exynos_acpm *exynos_acpm,
> + struct exynos_acpm_chan *chan)
> +{
> + struct device *dev = exynos_acpm->dev;
> + struct exynos_acpm_rx_data *rx_data;
> + unsigned int mlen = chan->mlen;
> + int i;
> +
> + for (i = 0; i < EXYNOS_ACPM_SEQNUM_MAX; i++) {
> + rx_data = &chan->rx_data[i];
> + rx_data->cmd = devm_kcalloc(dev, mlen, sizeof(*(rx_data->cmd)),
> + GFP_KERNEL);
> + if (!rx_data->cmd)
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static int exynos_acpm_chans_init(struct exynos_acpm *exynos_acpm)
> +{
> + struct exynos_acpm_shmem_chan *shmem_chans, *shmem_chan;
> + struct exynos_acpm_shmem *shmem = exynos_acpm->shmem;
> + struct mbox_chan *mbox_chan, *mbox_chans;
> + struct exynos_acpm_chan *chan, *chans;
> + struct device *dev = exynos_acpm->dev;
> + int i, ret;
> +
> + exynos_acpm->num_chans = readl_relaxed(&shmem->num_chans);
> +
> + mbox_chans = devm_kcalloc(dev, exynos_acpm->num_chans,
> + sizeof(*mbox_chans), GFP_KERNEL);
> + if (!mbox_chans)
> + return -ENOMEM;
> +
> + chans = devm_kcalloc(dev, exynos_acpm->num_chans, sizeof(*chans),
> + GFP_KERNEL);
> + if (!chans)
> + return -ENOMEM;
> +
> + shmem_chans = exynos_acpm_get_iomem_addr(exynos_acpm->sram_base,
> + &shmem->chans);
> +
> + for (i = 0; i < exynos_acpm->num_chans; i++) {
> + shmem_chan = &shmem_chans[i];
> + mbox_chan = &mbox_chans[i];
> + chan = &chans[i];
> +
> + /* Set parameters for the mailbox channel. */
> + mbox_chan->con_priv = chan;
> + mbox_chan->mbox = exynos_acpm->mbox;
> +
> + /* Set parameters for the ACPM channel. */
> + chan->mlen = readl_relaxed(&shmem_chan->mlen);
> +
> + ret = exynos_acpm_alloc_cmds(exynos_acpm, chan);
> + if (ret)
> + return ret;
> +
> + chan->polling = readl_relaxed(&shmem_chan->polling);
> + chan->id = readl_relaxed(&shmem_chan->id);
> + chan->qlen = readl_relaxed(&shmem_chan->qlen);
> +
> + exynos_acpm_rx_queue_init(exynos_acpm, shmem_chan, &chan->rx);
> + exynos_acpm_tx_queue_init(exynos_acpm, shmem_chan, &chan->tx);
> +
> + mutex_init(&chan->rx_lock);
> + spin_lock_init(&chan->tx_lock);
> +
> + dev_vdbg(dev, "ID = %d poll = %d, mlen = %d, qlen = %d\n",
> + chan->id, chan->polling, chan->mlen, chan->qlen);
> + }
> +
> + /* Save pointers to the ACPM and mailbox channels. */
> + exynos_acpm->mbox->chans = mbox_chans;
> + exynos_acpm->chans = chans;
> +
> + return 0;
> +}
> +
> +static const struct of_device_id exynos_acpm_match[] = {
> + { .compatible = "google,gs101-acpm" },
Where are the bindings?
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, exynos_acpm_match);
> +
> +static int exynos_acpm_probe(struct platform_device *pdev)
> +{
> + struct device_node *node = pdev->dev.of_node;
> + struct device *dev = &pdev->dev;
> + struct exynos_acpm *exynos_acpm;
> + struct mbox_controller *mbox;
> + struct device_node *shmem;
> + resource_size_t size;
> + struct resource res;
> + const __be32 *prop;
> + int ret;
> +
> + exynos_acpm = devm_kzalloc(dev, sizeof(*exynos_acpm), GFP_KERNEL);
> + if (!exynos_acpm)
> + return -ENOMEM;
> +
> + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> + if (!mbox)
> + return -ENOMEM;
> +
> + exynos_acpm->regs = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(exynos_acpm->regs))
> + return PTR_ERR(exynos_acpm->regs);
> +
> + shmem = of_parse_phandle(node, "shmem", 0);
> + ret = of_address_to_resource(shmem, 0, &res);
> + of_node_put(shmem);
> + if (ret) {
> + dev_err(dev, "Failed to get shared memory.\n");
> + return ret;
> + }
> +
> + size = resource_size(&res);
> + exynos_acpm->sram_base = devm_ioremap(dev, res.start, size);
> + if (!exynos_acpm->sram_base) {
> + dev_err(dev, "Failed to ioremap shared memory.\n");
> + return -ENOMEM;
> + }
> +
> + prop = of_get_property(node, "initdata-base", NULL);
> + if (!prop) {
> + dev_err(dev, "Parsing initdata_base failed.\n");
> + return -EINVAL;
> + }
> +
> + exynos_acpm->pclk = devm_clk_get(dev, "pclk");
devm_clk_get_enabled
> + if (IS_ERR(exynos_acpm->pclk)) {
> + dev_err(dev, "Missing peripheral clock.\n");
return dev_err_probe()
> + return PTR_ERR(exynos_acpm->pclk);
> + }
> +
> + ret = clk_prepare_enable(exynos_acpm->pclk);
> + if (ret) {
> + dev_err(dev, "Failed to enable the peripheral clock.\n");
> + return ret;
> + }
> +
> + exynos_acpm->wq = alloc_workqueue("exynos-acpm-wq", 0, 0);
> + if (!exynos_acpm->wq)
> + return -ENOMEM;
> +
> + exynos_acpm->dev = dev;
> + exynos_acpm->mbox = mbox;
> + exynos_acpm->shmem = exynos_acpm->sram_base + be32_to_cpup(prop);
> +
> + ret = exynos_acpm_chans_init(exynos_acpm);
> + if (ret)
> + return ret;
> +
> + mbox->num_chans = exynos_acpm->num_chans;
> + mbox->dev = dev;
> + mbox->ops = &exynos_acpm_chan_ops;
> +
> + platform_set_drvdata(pdev, exynos_acpm);
> +
> + /* Mask out all interrupts. We support just polling channels for now. */
> + writel_relaxed(EXYNOS_ACPM_INTMR0_MASK,
> + exynos_acpm->regs + EXYNOS_ACPM_INTMR0);
> +
> + ret = devm_mbox_controller_register(dev, mbox);
> + if (ret)
> + dev_err(dev, "Failed to register mbox_controller(%d).\n", ret);
> +
> + return ret;
> +}
> +
> +static void exynos_acpm_remove(struct platform_device *pdev)
> +{
> + struct exynos_acpm *exynos_acpm = platform_get_drvdata(pdev);
> +
> + flush_workqueue(exynos_acpm->wq);
> + destroy_workqueue(exynos_acpm->wq);
> + clk_disable_unprepare(exynos_acpm->pclk);
> +}
> +
> +static struct platform_driver exynos_acpm_driver = {
> + .probe = exynos_acpm_probe,
> + .remove = exynos_acpm_remove,
> + .driver = {
> + .name = "exynos-acpm",
> + .owner = THIS_MODULE,
Drop
> + .of_match_table = exynos_acpm_match,
> + },
> +};
> +module_platform_driver(exynos_acpm_driver);
Best regards,
Krzysztof
Hi, Krzysztof,
On 10/21/24 12:52 PM, Krzysztof Kozlowski wrote:
> On 17/10/2024 18:36, Tudor Ambarus wrote:
>> 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.
>>
>> The mailbox channels are initialized based on the configuration data
>> found at a specific offset into the shared memory (mmio-sram). The
>> configuration data consists of channel id, message and queue lengths,
>> pointers to the RX and TX queues which are also part of the SRAM, and
>> whether RX works by polling or interrupts. All known clients of this
>> driver are using polling channels, thus the driver implements for now
>> just polling mode.
>>
>> Add support for the exynos acpm core driver. Helper drivers will follow.
>> These will construct the mailbox messages in the format expected by the
>> firmware.
>
> I skimmed through the driver and I do not understand why this is
> firmware. You are implementing a mailbox provider/controller.
In my case the mailbox hardware is used just to raise the interrupt to
the other side. Then there's the SRAM which contains the channels
configuration data and the TX/RX queues. The enqueue/deque is done
in/from SRAM. This resembles a lot with drivers/firmware/arm_scmi/, see:
drivers/firmware/arm_scmi/shmem.c
drivers/firmware/arm_scmi/transports/mailbox.c
After the SRAM and mailbox/transport code I'll come up with two helper
drivers that construct the mailbox messages in the format expected by
the firmware. There are 2 types of messages recognized by the ACPM
firmware: PMIC and DVFS. The client drivers will use these helper
drivers to prepare a specific message. Then they will use the mailbox
core to send the message and they'll wait for the answer.
This layered structure and the use of SRAM resembles with arm_scmi and
made me think that the ACPM driver it's better suited for
drivers/firmware. I'm opened for suggestions though.
>
> I did not perform full review yet, just skimmed over the code. I will
> take a look a bit later.
>
No worries, thanks for doing it. I agree with all the suggestions from
below and I'll address them after we get an agreement with Jassi on how
to extend the mailbox core.
More answers below.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>> drivers/firmware/Kconfig | 1 +
>> drivers/firmware/Makefile | 1 +
>> drivers/firmware/samsung/Kconfig | 11 +
>> drivers/firmware/samsung/Makefile | 3 +
>> drivers/firmware/samsung/exynos-acpm.c | 703 ++++++++++++++++++++
>
> Please add directory to the Samsung Exynos SoC maintainer entry. I also
> encourage adding separate entry for the driver where you would be listed
> as maintainer.
ok
>
> There is no firmware tree, so this will be going via Samsung SoC.
I noticed afterwards, thanks.
>
>> include/linux/mailbox/exynos-acpm-message.h | 21 +
>> 6 files changed, 740 insertions(+)
>> create mode 100644 drivers/firmware/samsung/Kconfig
>> create mode 100644 drivers/firmware/samsung/Makefile
>> create mode 100644 drivers/firmware/samsung/exynos-acpm.c
>> create mode 100644 include/linux/mailbox/exynos-acpm-message.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..f908773c1441
>> --- /dev/null
>> +++ b/drivers/firmware/samsung/Kconfig
>> @@ -0,0 +1,11 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +
>> +config EXYNOS_ACPM
>> + tristate "Exynos ACPM (Alive Clock and Power Manager) driver support"
>
> depends ARCH_EXYNOS || COMPILE_TEST
oh yes.
>
> Please also send a arm64 defconfig change making it a module.
will do
cut
>> +
>> +/**
>> + * struct exynos_acpm_shmem_chan - descriptor of a shared memory channel.
>> + *
>> + * @id: channel ID.
>> + * @reserved: reserved for future use.
>> + * @rx_rear: rear pointer of RX queue.
>> + * @rx_front: front pointer of RX queue.
>> + * @rx_base: base address of RX queue.
>> + * @reserved1: reserved for future use.
>> + * @tx_rear: rear pointer of TX queue.
>> + * @tx_front: front pointer of TX queue.
>> + * @tx_base: base address of TX queue.
>> + * @qlen: queue length. Applies to both TX/RX queues.
>> + * @mlen: message length. Applies to both TX/RX queues.
>> + * @reserved2: reserved for future use.
>> + * @polling: true when the channel works on polling.
>> + */
>> +struct exynos_acpm_shmem_chan {
>> + 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 polling;
>
> Why are you storing addresses as u32? Shouldn't these be __iomem*?
This structure defines the offsets in SRAM that describe the channel
parameters. Instances of this struct shall be declared indeed as:
struct exynos_acpm_shmem_chan __iomem *shmem_chan;
I missed that in v2, but will update in v2.
>
> I also cannot find any piece of code setting several of above, e.g. tx_base
I'm not writing any SRAM configuration fields, these fields are used to
read/retrive the channel parameters from SRAM.
cut
>> +
>> + spin_lock_irqsave(&chan->tx_lock, flags);
>> +
>> + tx_front = readl_relaxed(chan->tx.front);
>> + idx = (tx_front + 1) % chan->qlen;
>> +
>> + ret = exynos_acpm_wait_for_queue_slots(mbox_chan, idx);
>> + if (ret)
>> + goto exit;
>> +
>> + exynos_acpm_prepare_request(mbox_chan, req);
>> +
>> + /* Write TX command. */
>> + __iowrite32_copy(chan->tx.base + chan->mlen * tx_front, tx->cmd,
>> + req->txlen / 4);
>> +
>> + /* Advance TX front. */
>> + writel_relaxed(idx, chan->tx.front);
>> +
>> + /* Flush SRAM posted writes. */
>> + readl_relaxed(chan->tx.front);
>> +
>
> How does this flush work? Maybe you just miss here barries (s/relaxed//)?
I think _relaxed() accessors should be fine in this driver as there are
no DMA accesses involved. _relaxed() accessors are fully ordered for
accesses to the same device/endpoint.
I'm worried however about the posted writes, the buses the devices sit
on may themselves have asynchronicity. So I issue a read from the same
device to ensure that the write has occured.
There is something that I haven't dimistified though. You'll notice that
the writes from above are on SRAM. I enqueue on the TX queue then
advance the head/front of the queue and then I read back to make sure
that the writes occured. Below I write to the controller's interrupt
register (different device/endpoint) to raise the interrupt for the
counterpart and inform that TX queue advanced. I'm not sure whether I
need a barrier here to make sure that the CPU does not reorder the
accesses and raise the interrupt before advancing the TX queue. If
someone already knows the answer, please say, I'll do some more reading
in the meantime.
>
>> + /* Generate ACPM interrupt. */
>> + writel_relaxed(BIT(chan->id), exynos_acpm->regs + EXYNOS_ACPM_INTGR1);
>> +
>> + /* Flush mailbox controller posted writes. */
>> + readl_relaxed(exynos_acpm->regs + EXYNOS_ACPM_MCUCTRL);
>> +
>> + spin_unlock_irqrestore(&chan->tx_lock, flags);
>> +
>> + queue_work(exynos_acpm->wq, &work_data->work);
>> +
>> + return -EINPROGRESS;
>> +exit:
>> + spin_unlock_irqrestore(&chan->tx_lock, flags);
>> + kfree(work_data);
>> + return ret;
>> +}
cut
>> +static const struct of_device_id exynos_acpm_match[] = {
>> + { .compatible = "google,gs101-acpm" },
>
> Where are the bindings?
will follow soon.
cut
>> +static int exynos_acpm_probe(struct platform_device *pdev)
>> +{
cut
>> + exynos_acpm->pclk = devm_clk_get(dev, "pclk");
>
> devm_clk_get_enabled
ok
>
>> + if (IS_ERR(exynos_acpm->pclk)) {
>> + dev_err(dev, "Missing peripheral clock.\n");
>
> return dev_err_probe()
ok
cut
>> + .owner = THIS_MODULE,
>
> Drop
oh yes, thanks!
ta
On 21/10/2024 16:12, Tudor Ambarus wrote:
> Hi, Krzysztof,
>
> On 10/21/24 12:52 PM, Krzysztof Kozlowski wrote:
>> On 17/10/2024 18:36, Tudor Ambarus wrote:
>>> 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.
>>>
>>> The mailbox channels are initialized based on the configuration data
>>> found at a specific offset into the shared memory (mmio-sram). The
>>> configuration data consists of channel id, message and queue lengths,
>>> pointers to the RX and TX queues which are also part of the SRAM, and
>>> whether RX works by polling or interrupts. All known clients of this
>>> driver are using polling channels, thus the driver implements for now
>>> just polling mode.
>>>
>>> Add support for the exynos acpm core driver. Helper drivers will follow.
>>> These will construct the mailbox messages in the format expected by the
>>> firmware.
>>
>> I skimmed through the driver and I do not understand why this is
>> firmware. You are implementing a mailbox provider/controller.
>
> In my case the mailbox hardware is used just to raise the interrupt to
> the other side. Then there's the SRAM which contains the channels
> configuration data and the TX/RX queues. The enqueue/deque is done
> in/from SRAM. This resembles a lot with drivers/firmware/arm_scmi/, see:
>
> drivers/firmware/arm_scmi/shmem.c
> drivers/firmware/arm_scmi/transports/mailbox.c
Wait, SCMI is an interface. Not the case here.
>
> After the SRAM and mailbox/transport code I'll come up with two helper
> drivers that construct the mailbox messages in the format expected by
> the firmware. There are 2 types of messages recognized by the ACPM
> firmware: PMIC and DVFS. The client drivers will use these helper
> drivers to prepare a specific message. Then they will use the mailbox
> core to send the message and they'll wait for the answer.
>
> This layered structure and the use of SRAM resembles with arm_scmi and
> made me think that the ACPM driver it's better suited for
> drivers/firmware. I'm opened for suggestions though.
Sure, but then this driver cannot perform mbox_controller_register().
Only mailbox providers, so drivers in mailbox, use it.
>
>>
>> I did not perform full review yet, just skimmed over the code. I will
>> take a look a bit later.
>>
>
> No worries, thanks for doing it. I agree with all the suggestions from
> below and I'll address them after we get an agreement with Jassi on how
> to extend the mailbox core.
>
> More answers below.
>
>>>
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>>> ---
>>> drivers/firmware/Kconfig | 1 +
>>> drivers/firmware/Makefile | 1 +
>>> drivers/firmware/samsung/Kconfig | 11 +
>>> drivers/firmware/samsung/Makefile | 3 +
>>> drivers/firmware/samsung/exynos-acpm.c | 703 ++++++++++++++++++++
>>
>> Please add directory to the Samsung Exynos SoC maintainer entry. I also
>> encourage adding separate entry for the driver where you would be listed
>> as maintainer.
>
> ok
>
>>
>> There is no firmware tree, so this will be going via Samsung SoC.
>
> I noticed afterwards, thanks.
>
>>
>>> include/linux/mailbox/exynos-acpm-message.h | 21 +
>>> 6 files changed, 740 insertions(+)
>>> create mode 100644 drivers/firmware/samsung/Kconfig
>>> create mode 100644 drivers/firmware/samsung/Makefile
>>> create mode 100644 drivers/firmware/samsung/exynos-acpm.c
>>> create mode 100644 include/linux/mailbox/exynos-acpm-message.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..f908773c1441
>>> --- /dev/null
>>> +++ b/drivers/firmware/samsung/Kconfig
>>> @@ -0,0 +1,11 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +config EXYNOS_ACPM
>>> + tristate "Exynos ACPM (Alive Clock and Power Manager) driver support"
>>
>> depends ARCH_EXYNOS || COMPILE_TEST
>
> oh yes.
>>
>> Please also send a arm64 defconfig change making it a module.
>
> will do
>
> cut
>
>>> +
>>> +/**
>>> + * struct exynos_acpm_shmem_chan - descriptor of a shared memory channel.
>>> + *
>>> + * @id: channel ID.
>>> + * @reserved: reserved for future use.
>>> + * @rx_rear: rear pointer of RX queue.
>>> + * @rx_front: front pointer of RX queue.
>>> + * @rx_base: base address of RX queue.
>>> + * @reserved1: reserved for future use.
>>> + * @tx_rear: rear pointer of TX queue.
>>> + * @tx_front: front pointer of TX queue.
>>> + * @tx_base: base address of TX queue.
>>> + * @qlen: queue length. Applies to both TX/RX queues.
>>> + * @mlen: message length. Applies to both TX/RX queues.
>>> + * @reserved2: reserved for future use.
>>> + * @polling: true when the channel works on polling.
>>> + */
>>> +struct exynos_acpm_shmem_chan {
>>> + 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 polling;
>>
>> Why are you storing addresses as u32? Shouldn't these be __iomem*?
>
> This structure defines the offsets in SRAM that describe the channel
> parameters. Instances of this struct shall be declared indeed as:
> struct exynos_acpm_shmem_chan __iomem *shmem_chan;
> I missed that in v2, but will update in v2.
>
>>
>> I also cannot find any piece of code setting several of above, e.g. tx_base
>
> I'm not writing any SRAM configuration fields, these fields are used to
> read/retrive the channel parameters from SRAM.
I meany tx_base is always 0. Where is this property set? Ever?
>
> cut
>
>>> +
>>> + spin_lock_irqsave(&chan->tx_lock, flags);
>>> +
>>> + tx_front = readl_relaxed(chan->tx.front);
>>> + idx = (tx_front + 1) % chan->qlen;
>>> +
>>> + ret = exynos_acpm_wait_for_queue_slots(mbox_chan, idx);
>>> + if (ret)
>>> + goto exit;
>>> +
>>> + exynos_acpm_prepare_request(mbox_chan, req);
>>> +
>>> + /* Write TX command. */
>>> + __iowrite32_copy(chan->tx.base + chan->mlen * tx_front, tx->cmd,
>>> + req->txlen / 4);
>>> +
>>> + /* Advance TX front. */
>>> + writel_relaxed(idx, chan->tx.front);
>>> +
>>> + /* Flush SRAM posted writes. */
>>> + readl_relaxed(chan->tx.front);
>>> +
>>
>> How does this flush work? Maybe you just miss here barries (s/relaxed//)?
>
> I think _relaxed() accessors should be fine in this driver as there are
> no DMA accesses involved. _relaxed() accessors are fully ordered for
> accesses to the same device/endpoint.
>
> I'm worried however about the posted writes, the buses the devices sit
> on may themselves have asynchronicity. So I issue a read from the same
> device to ensure that the write has occured.
Hm, ok, it seems it is actually standard way for posted bus.
>
> There is something that I haven't dimistified though. You'll notice that
> the writes from above are on SRAM. I enqueue on the TX queue then
> advance the head/front of the queue and then I read back to make sure
> that the writes occured. Below I write to the controller's interrupt
> register (different device/endpoint) to raise the interrupt for the
> counterpart and inform that TX queue advanced. I'm not sure whether I
> need a barrier here to make sure that the CPU does not reorder the
> accesses and raise the interrupt before advancing the TX queue. If
> someone already knows the answer, please say, I'll do some more reading
> in the meantime.
I think it is fine.
Best regards,
Krzysztof
Hi, Krzysztof,
On 10/22/24 5:38 AM, Krzysztof Kozlowski wrote:
cut
>>> I skimmed through the driver and I do not understand why this is
>>> firmware. You are implementing a mailbox provider/controller.
>>
>> In my case the mailbox hardware is used just to raise the interrupt to
>> the other side. Then there's the SRAM which contains the channels
>> configuration data and the TX/RX queues. The enqueue/deque is done
>> in/from SRAM. This resembles a lot with drivers/firmware/arm_scmi/, see:
>>
>> drivers/firmware/arm_scmi/shmem.c
>> drivers/firmware/arm_scmi/transports/mailbox.c
>
> Wait, SCMI is an interface. Not the case here.
>
>>
>> After the SRAM and mailbox/transport code I'll come up with two helper
>> drivers that construct the mailbox messages in the format expected by
>> the firmware. There are 2 types of messages recognized by the ACPM
>> firmware: PMIC and DVFS. The client drivers will use these helper
>> drivers to prepare a specific message. Then they will use the mailbox
>> core to send the message and they'll wait for the answer.
>>
>> This layered structure and the use of SRAM resembles with arm_scmi and
>> made me think that the ACPM driver it's better suited for
>> drivers/firmware. I'm opened for suggestions though.
>
> Sure, but then this driver cannot perform mbox_controller_register().
> Only mailbox providers, so drivers in mailbox, use it.
>
Okay, I can move the driver to drivers/mailbox/.
cut
>>>> +/**
>>>> + * struct exynos_acpm_shmem_chan - descriptor of a shared memory channel.
>>>> + *
>>>> + * @id: channel ID.
>>>> + * @reserved: reserved for future use.
>>>> + * @rx_rear: rear pointer of RX queue.
>>>> + * @rx_front: front pointer of RX queue.
>>>> + * @rx_base: base address of RX queue.
>>>> + * @reserved1: reserved for future use.
>>>> + * @tx_rear: rear pointer of TX queue.
>>>> + * @tx_front: front pointer of TX queue.
>>>> + * @tx_base: base address of TX queue.
>>>> + * @qlen: queue length. Applies to both TX/RX queues.
>>>> + * @mlen: message length. Applies to both TX/RX queues.
>>>> + * @reserved2: reserved for future use.
>>>> + * @polling: true when the channel works on polling.
>>>> + */
>>>> +struct exynos_acpm_shmem_chan {
>>>> + 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 polling;
>>>
cut
>>>
>>> I also cannot find any piece of code setting several of above, e.g. tx_base
>>
>> I'm not writing any SRAM configuration fields, these fields are used to
>> read/retrive the channel parameters from SRAM.
>
> I meany tx_base is always 0. Where is this property set? Ever?
It's not zero. My assumption is it is set in the acpm firmware, but I
don't have access to that to verify. Here are some debug prints made in
the linux driver:
[ 0.069575][ T1] gs-acpm-ipc 17610000.mailbox:
exynos_mbox_chan_init ID = 2 poll = 1, mlen = 16, qlen = 5
[ 0.069927][ T1] gs-acpm-ipc 17610000.mailbox:
exynos_mbox_chan_init ID = 2 offsets: rx_base = 0x00038290 rx_front =
0x0003828c, rx_rear = 0x00038288
[ 0.070449][ T1] gs-acpm-ipc 17610000.mailbox:
exynos_mbox_chan_init ID = 2 offsets: tx_base = 0x000382f0 tx_front =
0x000382ec, tx_rear = 0x000382e8
tx_base contains the SRAM offset of the RX queue used in linux. The
offset is relative to the base address of the SRAM config data.
tx_base is seen/named from the firmware's point of view, thus named TX.
I assume the same struct is defined in the acpm firmware.
Somewhere below in the linux driver I get the RX ring base address by doing:
rx->base = exynos_acpm_get_iomem_addr(base, &shmem_chan->tx_base);
where base is the SRAM base address of the channels configuration data.
static void __iomem *exynos_acpm_get_iomem_addr(void __iomem *base,
void __iomem *addr)
{
u32 offset;
offset = readl_relaxed(addr);
return base + offset;
}
Hope this clarifies a bit these struct members.
Cheers,
ta
On 22/10/2024 09:58, Tudor Ambarus wrote:
>
>>>>
>>>> I also cannot find any piece of code setting several of above, e.g. tx_base
>>>
>>> I'm not writing any SRAM configuration fields, these fields are used to
>>> read/retrive the channel parameters from SRAM.
>>
>> I meany tx_base is always 0. Where is this property set? Ever?
>
> It's not zero. My assumption is it is set in the acpm firmware, but I
Where is any assignment to this member?
> don't have access to that to verify. Here are some debug prints made in
> the linux driver:
>
> [ 0.069575][ T1] gs-acpm-ipc 17610000.mailbox:
> exynos_mbox_chan_init ID = 2 poll = 1, mlen = 16, qlen = 5
> [ 0.069927][ T1] gs-acpm-ipc 17610000.mailbox:
> exynos_mbox_chan_init ID = 2 offsets: rx_base = 0x00038290 rx_front =
> 0x0003828c, rx_rear = 0x00038288
> [ 0.070449][ T1] gs-acpm-ipc 17610000.mailbox:
> exynos_mbox_chan_init ID = 2 offsets: tx_base = 0x000382f0 tx_front =
> 0x000382ec, tx_rear = 0x000382e8
>
>
> tx_base contains the SRAM offset of the RX queue used in linux. The
> offset is relative to the base address of the SRAM config data.
>
> tx_base is seen/named from the firmware's point of view, thus named TX.
> I assume the same struct is defined in the acpm firmware.
>
>
> Somewhere below in the linux driver I get the RX ring base address by doing:
>
> rx->base = exynos_acpm_get_iomem_addr(base, &shmem_chan->tx_base);
tx_base is still 0.
>
> where base is the SRAM base address of the channels configuration data.
>
> static void __iomem *exynos_acpm_get_iomem_addr(void __iomem *base,
>
>
> void __iomem *addr)
>
>
> {
>
>
> u32 offset;
>
>
>
>
>
> offset = readl_relaxed(addr);
>
>
> return base + offset;
>
>
> }
>
> Hope this clarifies a bit these struct members.
No, where is tx_base assigned?
Best regards,
Krzysztof
On 10/23/24 10:00 AM, Krzysztof Kozlowski wrote:
>>>>> I also cannot find any piece of code setting several of above, e.g. tx_base
>>>> I'm not writing any SRAM configuration fields, these fields are used to
>>>> read/retrive the channel parameters from SRAM.
>>> I meany tx_base is always 0. Where is this property set? Ever?
>> It's not zero. My assumption is it is set in the acpm firmware, but I
> Where is any assignment to this member?
In probe() you'll see that exynos_acpm->shmem is a pointer in SRAM to a
struct exynos_acpm_shmem __iomem *shmem;
Then in:
static int exynos_acpm_chans_init()
{
struct exynos_acpm_shmem_chan __iomem *shmem_chans, *shmem_chan;
struct exynos_acpm_shmem __iomem *shmem = exynos_acpm->shmem;
...
shmem_chans = exynos_acpm_get_iomem_addr(exynos_acpm->sram_base,
&shmem->chans);
...
}
shmem->chans is not initialized (or tx_base). I'm using its address in
SRAM (&shmem->chans) which I then read it with readl_relaxed().
I guess one can do the same using offsetof:
shmem_chans = readl_realaxed(shmem + offsetof(struct exynos_acpm_shmem,
chans));
Thanks,
ta
On 23/10/2024 11:53, Tudor Ambarus wrote:
>
>
> On 10/23/24 10:00 AM, Krzysztof Kozlowski wrote:
>>>>>> I also cannot find any piece of code setting several of above, e.g. tx_base
>>>>> I'm not writing any SRAM configuration fields, these fields are used to
>>>>> read/retrive the channel parameters from SRAM.
>>>> I meany tx_base is always 0. Where is this property set? Ever?
>>> It's not zero. My assumption is it is set in the acpm firmware, but I
>> Where is any assignment to this member?
>
> In probe() you'll see that exynos_acpm->shmem is a pointer in SRAM to a
> struct exynos_acpm_shmem __iomem *shmem;
>
> Then in:
>
> static int exynos_acpm_chans_init()
> {
> struct exynos_acpm_shmem_chan __iomem *shmem_chans, *shmem_chan;
> struct exynos_acpm_shmem __iomem *shmem = exynos_acpm->shmem;
> ...
>
> shmem_chans = exynos_acpm_get_iomem_addr(exynos_acpm->sram_base,
> &shmem->chans);
> ...
> }
>
> shmem->chans is not initialized (or tx_base). I'm using its address in
> SRAM (&shmem->chans) which I then read it with readl_relaxed().
>
> I guess one can do the same using offsetof:
> shmem_chans = readl_realaxed(shmem + offsetof(struct exynos_acpm_shmem,
> chans));
>
I see, the code and the naming is confusing. Two exynos_acpm_shmem_chan
variables and one exynos_acpm_shmem. shmem_chans is used as an array,
but nowhere pointed or indicated that it is array of some size.
All this could be clearer if exynos_acpm_shmem_chan was packed, because
then it is obvious it points to defined memory, but maybe packed is not
correct here? Probably splitting all this into logical chunks would be
useful. Like not mixing reading offsets with reading values, because I
really have to spend a lot of time to identify which one is which in
exynos_acpm_chans_init().
Best regards,
Krzysztof
On 10/24/24 10:36 AM, Krzysztof Kozlowski wrote:
> On 23/10/2024 11:53, Tudor Ambarus wrote:
>>
>>
>> On 10/23/24 10:00 AM, Krzysztof Kozlowski wrote:
>>>>>>> I also cannot find any piece of code setting several of above, e.g. tx_base
>>>>>> I'm not writing any SRAM configuration fields, these fields are used to
>>>>>> read/retrive the channel parameters from SRAM.
>>>>> I meany tx_base is always 0. Where is this property set? Ever?
>>>> It's not zero. My assumption is it is set in the acpm firmware, but I
>>> Where is any assignment to this member?
>>
>> In probe() you'll see that exynos_acpm->shmem is a pointer in SRAM to a
>> struct exynos_acpm_shmem __iomem *shmem;
>>
>> Then in:
>>
>> static int exynos_acpm_chans_init()
>> {
>> struct exynos_acpm_shmem_chan __iomem *shmem_chans, *shmem_chan;
>> struct exynos_acpm_shmem __iomem *shmem = exynos_acpm->shmem;
>> ...
>>
>> shmem_chans = exynos_acpm_get_iomem_addr(exynos_acpm->sram_base,
>> &shmem->chans);
>> ...
>> }
>>
>> shmem->chans is not initialized (or tx_base). I'm using its address in
>> SRAM (&shmem->chans) which I then read it with readl_relaxed().
>>
>> I guess one can do the same using offsetof:
>> shmem_chans = readl_realaxed(shmem + offsetof(struct exynos_acpm_shmem,
>> chans));
>>
>
> I see, the code and the naming is confusing. Two exynos_acpm_shmem_chan
Noted. I'll refactor exynos_acpm_chans_init() in the next version.
> variables and one exynos_acpm_shmem. shmem_chans is used as an array,
> but nowhere pointed or indicated that it is array of some size.
>
I understand , will update. I added documentation for `struct
exynos_acpm_shmem` describing the array of chans and the number of
chans, but I'll figure something more, to be clearer.
> All this could be clearer if exynos_acpm_shmem_chan was packed, because
> then it is obvious it points to defined memory, but maybe packed is not
__packed shall be alright, but it's not needed because all the members
of the struct are u32 and the address of the struct is u64 aligned.
> correct here? Probably splitting all this into logical chunks would be
> useful. Like not mixing reading offsets with reading values, because I
> really have to spend a lot of time to identify which one is which in
> exynos_acpm_chans_init().
>
I understand, will update. Need to figure out what other options we have
with the mailbox core changes first. Thanks for the suggestions!
Cheers,
ta
On 10/23/24 10:53 AM, Tudor Ambarus wrote:
>
>
> On 10/23/24 10:00 AM, Krzysztof Kozlowski wrote:
>>>>>> I also cannot find any piece of code setting several of above, e.g. tx_base
>>>>> I'm not writing any SRAM configuration fields, these fields are used to
>>>>> read/retrive the channel parameters from SRAM.
>>>> I meany tx_base is always 0. Where is this property set? Ever?
>>> It's not zero. My assumption is it is set in the acpm firmware, but I
>> Where is any assignment to this member?
>
> In probe() you'll see that exynos_acpm->shmem is a pointer in SRAM to a
> struct exynos_acpm_shmem __iomem *shmem;
>
> Then in:
>
> static int exynos_acpm_chans_init()
> {
> struct exynos_acpm_shmem_chan __iomem *shmem_chans, *shmem_chan;
> struct exynos_acpm_shmem __iomem *shmem = exynos_acpm->shmem;
> ...
>
> shmem_chans = exynos_acpm_get_iomem_addr(exynos_acpm->sram_base,
> &shmem->chans);
> ...
> }
>
> shmem->chans is not initialized (or tx_base). I'm using its address in
> SRAM (&shmem->chans) which I then read it with readl_relaxed().
>
> I guess one can do the same using offsetof:
> shmem_chans = readl_realaxed(shmem + offsetof(struct exynos_acpm_shmem,
> chans));
>
I forgot to add the sram_base, the counter example should have been:
shmem_chans = exynos_acpm->sram_base +
readl_realaxed(shmem + offsetof(struct exynos_acpm_shmem,
chans));
On Tue, 22 Oct 2024 at 06:38, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 21/10/2024 16:12, Tudor Ambarus wrote:
> > Hi, Krzysztof,
> >
> > On 10/21/24 12:52 PM, Krzysztof Kozlowski wrote:
> >> On 17/10/2024 18:36, Tudor Ambarus wrote:
> >>> 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.
> >>>
> >>> The mailbox channels are initialized based on the configuration data
> >>> found at a specific offset into the shared memory (mmio-sram). The
> >>> configuration data consists of channel id, message and queue lengths,
> >>> pointers to the RX and TX queues which are also part of the SRAM, and
> >>> whether RX works by polling or interrupts. All known clients of this
> >>> driver are using polling channels, thus the driver implements for now
> >>> just polling mode.
> >>>
> >>> Add support for the exynos acpm core driver. Helper drivers will follow.
> >>> These will construct the mailbox messages in the format expected by the
> >>> firmware.
> >>
> >> I skimmed through the driver and I do not understand why this is
> >> firmware. You are implementing a mailbox provider/controller.
> >
> > In my case the mailbox hardware is used just to raise the interrupt to
> > the other side. Then there's the SRAM which contains the channels
> > configuration data and the TX/RX queues. The enqueue/deque is done
> > in/from SRAM. This resembles a lot with drivers/firmware/arm_scmi/, see:
> >
> > drivers/firmware/arm_scmi/shmem.c
> > drivers/firmware/arm_scmi/transports/mailbox.c
>
> Wait, SCMI is an interface. Not the case here.
How is it different from SCMI ? They both use mailbox and shared
memory to set a message in the SRAM and ping the other side with the
mailbox. The only diff is that SCMI is an public spec whereas this one
is specific to some SoC
>
> >
> > After the SRAM and mailbox/transport code I'll come up with two helper
> > drivers that construct the mailbox messages in the format expected by
> > the firmware. There are 2 types of messages recognized by the ACPM
> > firmware: PMIC and DVFS. The client drivers will use these helper
> > drivers to prepare a specific message. Then they will use the mailbox
> > core to send the message and they'll wait for the answer.
> >
> > This layered structure and the use of SRAM resembles with arm_scmi and
> > made me think that the ACPM driver it's better suited for
> > drivers/firmware. I'm opened for suggestions though.
>
> Sure, but then this driver cannot perform mbox_controller_register().
> Only mailbox providers, so drivers in mailbox, use it.
Yes, the mailbox driver part should go into mailbox and this part
should then use mbox_request_channel() to get a channel
>
> >
> >>
> >> I did not perform full review yet, just skimmed over the code. I will
> >> take a look a bit later.
> >>
> >
> > No worries, thanks for doing it. I agree with all the suggestions from
> > below and I'll address them after we get an agreement with Jassi on how
> > to extend the mailbox core.
> >
> > More answers below.
> >
> >>>
> >>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> >>> ---
> >>> drivers/firmware/Kconfig | 1 +
> >>> drivers/firmware/Makefile | 1 +
> >>> drivers/firmware/samsung/Kconfig | 11 +
> >>> drivers/firmware/samsung/Makefile | 3 +
> >>> drivers/firmware/samsung/exynos-acpm.c | 703 ++++++++++++++++++++
> >>
> >> Please add directory to the Samsung Exynos SoC maintainer entry. I also
> >> encourage adding separate entry for the driver where you would be listed
> >> as maintainer.
> >
> > ok
> >
> >>
> >> There is no firmware tree, so this will be going via Samsung SoC.
> >
> > I noticed afterwards, thanks.
> >
> >>
> >>> include/linux/mailbox/exynos-acpm-message.h | 21 +
> >>> 6 files changed, 740 insertions(+)
> >>> create mode 100644 drivers/firmware/samsung/Kconfig
> >>> create mode 100644 drivers/firmware/samsung/Makefile
> >>> create mode 100644 drivers/firmware/samsung/exynos-acpm.c
> >>> create mode 100644 include/linux/mailbox/exynos-acpm-message.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..f908773c1441
> >>> --- /dev/null
> >>> +++ b/drivers/firmware/samsung/Kconfig
> >>> @@ -0,0 +1,11 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only
> >>> +
> >>> +config EXYNOS_ACPM
> >>> + tristate "Exynos ACPM (Alive Clock and Power Manager) driver support"
> >>
> >> depends ARCH_EXYNOS || COMPILE_TEST
> >
> > oh yes.
> >>
> >> Please also send a arm64 defconfig change making it a module.
> >
> > will do
> >
> > cut
> >
> >>> +
> >>> +/**
> >>> + * struct exynos_acpm_shmem_chan - descriptor of a shared memory channel.
> >>> + *
> >>> + * @id: channel ID.
> >>> + * @reserved: reserved for future use.
> >>> + * @rx_rear: rear pointer of RX queue.
> >>> + * @rx_front: front pointer of RX queue.
> >>> + * @rx_base: base address of RX queue.
> >>> + * @reserved1: reserved for future use.
> >>> + * @tx_rear: rear pointer of TX queue.
> >>> + * @tx_front: front pointer of TX queue.
> >>> + * @tx_base: base address of TX queue.
> >>> + * @qlen: queue length. Applies to both TX/RX queues.
> >>> + * @mlen: message length. Applies to both TX/RX queues.
> >>> + * @reserved2: reserved for future use.
> >>> + * @polling: true when the channel works on polling.
> >>> + */
> >>> +struct exynos_acpm_shmem_chan {
> >>> + 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 polling;
> >>
> >> Why are you storing addresses as u32? Shouldn't these be __iomem*?
> >
> > This structure defines the offsets in SRAM that describe the channel
> > parameters. Instances of this struct shall be declared indeed as:
> > struct exynos_acpm_shmem_chan __iomem *shmem_chan;
> > I missed that in v2, but will update in v2.
> >
> >>
> >> I also cannot find any piece of code setting several of above, e.g. tx_base
> >
> > I'm not writing any SRAM configuration fields, these fields are used to
> > read/retrive the channel parameters from SRAM.
>
> I meany tx_base is always 0. Where is this property set? Ever?
>
> >
> > cut
> >
> >>> +
> >>> + spin_lock_irqsave(&chan->tx_lock, flags);
> >>> +
> >>> + tx_front = readl_relaxed(chan->tx.front);
> >>> + idx = (tx_front + 1) % chan->qlen;
> >>> +
> >>> + ret = exynos_acpm_wait_for_queue_slots(mbox_chan, idx);
> >>> + if (ret)
> >>> + goto exit;
> >>> +
> >>> + exynos_acpm_prepare_request(mbox_chan, req);
> >>> +
> >>> + /* Write TX command. */
> >>> + __iowrite32_copy(chan->tx.base + chan->mlen * tx_front, tx->cmd,
> >>> + req->txlen / 4);
> >>> +
> >>> + /* Advance TX front. */
> >>> + writel_relaxed(idx, chan->tx.front);
> >>> +
> >>> + /* Flush SRAM posted writes. */
> >>> + readl_relaxed(chan->tx.front);
> >>> +
> >>
> >> How does this flush work? Maybe you just miss here barries (s/relaxed//)?
> >
> > I think _relaxed() accessors should be fine in this driver as there are
> > no DMA accesses involved. _relaxed() accessors are fully ordered for
> > accesses to the same device/endpoint.
> >
> > I'm worried however about the posted writes, the buses the devices sit
> > on may themselves have asynchronicity. So I issue a read from the same
> > device to ensure that the write has occured.
>
> Hm, ok, it seems it is actually standard way for posted bus.
>
> >
> > There is something that I haven't dimistified though. You'll notice that
> > the writes from above are on SRAM. I enqueue on the TX queue then
> > advance the head/front of the queue and then I read back to make sure
> > that the writes occured. Below I write to the controller's interrupt
> > register (different device/endpoint) to raise the interrupt for the
> > counterpart and inform that TX queue advanced. I'm not sure whether I
> > need a barrier here to make sure that the CPU does not reorder the
> > accesses and raise the interrupt before advancing the TX queue. If
> > someone already knows the answer, please say, I'll do some more reading
> > in the meantime.
>
> I think it is fine.
>
>
> Best regards,
> Krzysztof
>
© 2016 - 2024 Red Hat, Inc.